Ensure other types of SIMD related loads, stores, and indirections are marked as used with SIMD intrinsics

src/coreclr/jit/compiler.h

@@ -3758,6 +3758,7 @@ class Compiler
 
     GenTreeMDArr* gtNewMDArrLowerBound(GenTree* arrayOp, unsigned dim, unsigned rank);
 
+    void gtInitializeLclVarNode(GenTreeLclVarCommon* lclVar);
     void gtInitializeStoreNode(GenTree* store, GenTree* value);
 
     void gtInitializeIndirNode(GenTreeIndir* indir, GenTreeFlags indirFlags);
@@ -10177,8 +10178,6 @@ class Compiler
 
     GenTree* impSIMDPopStack();
 
-    void setLclRelatedToSIMDIntrinsic(GenTree* tree);
-
     // Get the size of the SIMD type in bytes
     int getSIMDTypeSizeInBytes(CORINFO_CLASS_HANDLE typeHnd)
     {

src/coreclr/jit/gentree.cpp

@@ -9769,6 +9769,7 @@ GenTreeLclVar* Compiler::gtNewStoreLclVarNode(unsigned lclNum, GenTree* value)
     }
 
     gtInitializeStoreNode(store, value);
+    gtInitializeLclVarNode(store);
 
     return store;
 }
@@ -9803,6 +9804,7 @@ GenTreeLclFld* Compiler::gtNewStoreLclFldNode(
     }
 
     gtInitializeStoreNode(store, value);
+    gtInitializeLclVarNode(store);
 
     return store;
 }
@@ -9907,31 +9909,14 @@ GenTreeCall* Compiler::gtNewCallNode(gtCallTypes           callType,
 
 GenTreeLclVar* Compiler::gtNewLclvNode(unsigned lnum, var_types type DEBUGARG(IL_OFFSET offs))
 {
-    assert(type != TYP_VOID);
-    // We need to ensure that all struct values are normalized.
-    // It might be nice to assert this in general, but we have stores of int to long.
-    if (varTypeIsStruct(type))
-    {
-        // Make an exception for implicit by-ref parameters during global morph, since
-        // their lvType has been updated to byref but their appearances have not yet all
-        // been rewritten and so may have struct type still.
-        LclVarDsc* varDsc = lvaGetDesc(lnum);
-
-        bool simd12ToSimd16Widening = false;
-#if defined(FEATURE_SIMD)
-        // We can additionally have a SIMD12 that was widened to a SIMD16, generally as part of lowering
-        simd12ToSimd16Widening = (type == TYP_SIMD16) && (varDsc->lvType == TYP_SIMD12);
-#endif // FEATURE_SIMD
-        assert((type == varDsc->lvType) || simd12ToSimd16Widening ||
-               (lvaIsImplicitByRefLocal(lnum) && fgGlobalMorph && (varDsc->lvType == TYP_BYREF)));
-    }
     GenTreeLclVar* node = new (this, GT_LCL_VAR) GenTreeLclVar(GT_LCL_VAR, type, lnum DEBUGARG(offs));
 
     /* Cannot have this assert because the inliner uses this function
      * to add temporaries */
 
     // assert(lnum < lvaCount);
 
+    gtInitializeLclVarNode(node);
     return node;
 }
 
@@ -9954,20 +9939,11 @@ GenTreeLclVar* Compiler::gtNewLclVarNode(unsigned lclNum, var_types type)
 
 GenTreeLclVar* Compiler::gtNewLclLNode(unsigned lnum, var_types type DEBUGARG(IL_OFFSET offs))
 {
-    // We need to ensure that all struct values are normalized.
-    // It might be nice to assert this in general, but we have stores of int to long.
-    if (varTypeIsStruct(type))
-    {
-        // Make an exception for implicit by-ref parameters during global morph, since
-        // their lvType has been updated to byref but their appearances have not yet all
-        // been rewritten and so may have struct type still.
-        assert(type == lvaTable[lnum].lvType ||
-               (lvaIsImplicitByRefLocal(lnum) && fgGlobalMorph && (lvaTable[lnum].lvType == TYP_BYREF)));
-    }
     // This local variable node may later get transformed into a large node
     assert(GenTree::s_gtNodeSizes[LargeOpOpcode()] > GenTree::s_gtNodeSizes[GT_LCL_VAR]);
     GenTreeLclVar* node =
         new (this, LargeOpOpcode()) GenTreeLclVar(GT_LCL_VAR, type, lnum DEBUGARG(offs) DEBUGARG(/*largeNode*/ true));
+    gtInitializeLclVarNode(node);
     return node;
 }
 
@@ -9980,6 +9956,7 @@ GenTreeLclFld* Compiler::gtNewLclVarAddrNode(unsigned lclNum, var_types type)
 GenTreeLclFld* Compiler::gtNewLclAddrNode(unsigned lclNum, unsigned lclOffs, var_types type)
 {
     GenTreeLclFld* node = new (this, GT_LCL_ADDR) GenTreeLclFld(GT_LCL_ADDR, type, lclNum, lclOffs);
+    gtInitializeLclVarNode(node);
     return node;
 }
 
@@ -10002,6 +9979,7 @@ GenTreeFieldList* Compiler::gtNewFieldList()
 GenTreeLclFld* Compiler::gtNewLclFldNode(unsigned lnum, var_types type, unsigned offset, ClassLayout* layout)
 {
     GenTreeLclFld* node = new (this, GT_LCL_FLD) GenTreeLclFld(GT_LCL_FLD, type, lnum, offset, layout);
+    gtInitializeLclVarNode(node);
     return node;
 }
 
@@ -10061,6 +10039,59 @@ GenTreeFieldAddr* Compiler::gtNewFieldAddrNode(var_types type, CORINFO_FIELD_HAN
     return fieldNode;
 }
 
+//------------------------------------------------------------------------
+// gtInitializeLclVarNode: Initialize a lcl var node.
+//
+// Common initialization for all lcl var nodes. Marks SIMD locals as "used in
+// a HW intrinsic".
+//
+// Arguments:
+//    lclVar - The lcl var node
+//
+void Compiler::gtInitializeLclVarNode(GenTreeLclVarCommon* lclVar)
+{
+#ifdef DEBUG
+    if (lclVar->OperIs(GT_LCL_VAR))
+    {
+        assert(!lclVar->TypeIs(TYP_VOID));
+
+        // We need to ensure that all struct values are normalized.
+        // It might be nice to assert this in general, but we have stores of int to long.
+        if (varTypeIsStruct(lclVar))
+        {
+            // Make an exception for implicit by-ref parameters during global morph, since
+            // their lvType has been updated to byref but their appearances have not yet all
+            // been rewritten and so may have struct type still.
+            LclVarDsc* varDsc = lvaGetDesc(lclVar);
+
+            bool simd12ToSimd16Widening = false;
+
+#if defined(FEATURE_SIMD)
+            // We can additionally have a SIMD12 that was widened to a SIMD16, generally as part of lowering
+            simd12ToSimd16Widening = lclVar->TypeIs(TYP_SIMD16) && (varDsc->lvType == TYP_SIMD12);
+#endif // FEATURE_SIMD
+
+            assert(lclVar->TypeIs(varDsc->lvType) || simd12ToSimd16Widening ||
+                   (lvaIsImplicitByRefLocal(lclVar->GetLclNum()) && fgGlobalMorph && (varDsc->lvType == TYP_BYREF)));
+        }
+    }
+#endif // DEBUG
+
+#if defined(FEATURE_SIMD)
+    var_types type = lclVar->TypeGet();
+
+    if (lclVar->IsLclVarAddr())
+    {
+        type = lvaGetDesc(lclVar)->lvType;
+    }
+
+    if (varTypeIsSIMDOrMask(type))
+    {
+        SetOpLclRelatedToSIMDIntrinsic(lclVar);
+    }
+#endif // FEATURE_SIMD
+}
+
 //------------------------------------------------------------------------
 // gtInitializeStoreNode: Initialize a store node.
 //
@@ -10079,15 +10110,12 @@ void Compiler::gtInitializeStoreNode(GenTree* store, GenTree* value)
 #if defined(FEATURE_SIMD)
     if (varTypeIsSIMDOrMask(store))
     {
-        // TODO-ASG: delete this zero-diff quirk.
-        if (!value->IsCall() || !value->AsCall()->ShouldHaveRetBufArg())
-        {
-            // We want to track SIMD/Mask stores as being intrinsics since they
-            // are functionally `mov` instructions and are more efficient when
-            // we don't promote, particularly when it occurs due to inlining.
-            SetOpLclRelatedToSIMDIntrinsic(store);
-            SetOpLclRelatedToSIMDIntrinsic(value);
-        }
+        // We don't want to mark the store if its an indir, since
+        // that pessimizes codegen "a lot". We do, however, still
+        // want to mark the value. For local stores, we will mark
+        // the store itself using gtInitializeLclVarNode
+
+        SetOpLclRelatedToSIMDIntrinsic(value);
     }
 #endif // FEATURE_SIMD
 }
@@ -10179,6 +10207,13 @@ GenTreeIndir* Compiler::gtNewIndir(var_types typ, GenTree* addr, GenTreeFlags in
     GenTreeIndir* indir = new (this, GT_IND) GenTreeIndir(GT_IND, typ, addr, nullptr);
     gtInitializeIndirNode(indir, indirFlags);
 
+#if defined(FEATURE_SIMD)
+    if (varTypeIsSIMDOrMask(typ))
+    {
+        SetOpLclRelatedToSIMDIntrinsic(addr);
+    }
+#endif // FEATURE_SIMD
+
     return indir;
 }
 
@@ -10230,6 +10265,7 @@ GenTreeBlk* Compiler::gtNewStoreBlkNode(ClassLayout* layout, GenTree* addr, GenT
 
     GenTreeBlk* store = new (this, GT_STORE_BLK) GenTreeBlk(GT_STORE_BLK, TYP_STRUCT, addr, value, layout);
     store->gtFlags |= GTF_ASG;
+
     gtInitializeIndirNode(store, indirFlags);
     gtInitializeStoreNode(store, value);
 
@@ -10254,6 +10290,7 @@ GenTreeStoreInd* Compiler::gtNewStoreIndNode(var_types type, GenTree* addr, GenT
 
     GenTreeStoreInd* store = new (this, GT_STOREIND) GenTreeStoreInd(type, addr, value);
     store->gtFlags |= GTF_ASG;
+
     gtInitializeIndirNode(store, indirFlags);
     gtInitializeStoreNode(store, value);
 
@@ -21893,9 +21930,44 @@ FieldSeq::FieldSeq(CORINFO_FIELD_HANDLE fieldHnd, ssize_t offset, FieldKind fiel
 //
 void Compiler::SetOpLclRelatedToSIMDIntrinsic(GenTree* op)
 {
-    if ((op != nullptr) && op->OperIsScalarLocal())
+    // We want to track SIMD/Mask accesses as being intrinsics since they
+    // are functionally `mov` instructions and are more efficient when
+    // we don't promote, particularly when it occurs due to inlining.
+    //
+    // Noting, however, we need to ensure this is really only called for
+    // indirections or local variables that are actually used as part of
+    // SIMD intrinsics, because it's purpose is to block promotion and it
+    // will pessimize codegen a lot if used incorrectly.
+    //
+    // With that regard, we expect this to be called for all operands of
+    // actual hwintrinsic nodes, for loads where the source is one of the
+    // supported TYP_SIMD or TYP_MASK, and for stores where the destination
+    // is one of the supported TYP_SIMD or TYP_MASK. We do not want it called
+    // just because the value being stored is TYP_SIMD or TYP_MASK, however.
+
+    if (op == nullptr)
+    {
+        return;
+    }
+    op = op->gtEffectiveVal();
+
+    if (op->OperIsIndir())
     {
-        setLclRelatedToSIMDIntrinsic(op);
+        op = op->AsIndir()->Addr()->gtEffectiveVal();
+    }
+
+    if (op->OperIsScalarLocal() || op->IsLclVarAddr())
+    {
+        // We don't currently mark LCL_FLD, STORE_LCL_FLD, or PHI_ARG
+
+        unsigned   lclNum    = op->AsLclVarCommon()->GetLclNum();
+        LclVarDsc* lclVarDsc = lvaGetDesc(lclNum);
+
+        if (!lclVarDsc->lvUsedInSIMDIntrinsic)
+        {
+            lclVarDsc->lvUsedInSIMDIntrinsic = true;
+            JITDUMP("Marked V%02u as used in SIMD intrinsic\n", lclNum);
+        }
     }
 }
 
@@ -25815,10 +25887,9 @@ GenTree* Compiler::gtNewSimdLoadNode(var_types type, GenTree* op1, var_types sim
     assert(getSIMDTypeForSize(simdSize) == type);
 
     assert(op1 != nullptr);
-
     assert(varTypeIsArithmetic(simdBaseType));
 
-    return gtNewIndir(type, op1);
+    return gtNewLoadValueNode(type, op1);
 }
 
 //----------------------------------------------------------------------------------------------
@@ -28540,12 +28611,12 @@ GenTree* Compiler::gtNewSimdStoreNode(GenTree* op1, GenTree* op2, var_types simd
     assert(op1 != nullptr);
     assert(op2 != nullptr);
 
+    assert(varTypeIsArithmetic(simdBaseType));
+
     assert(varTypeIsSIMD(op2));
     assert(getSIMDTypeForSize(simdSize) == op2->TypeGet());
 
-    assert(varTypeIsArithmetic(simdBaseType));
-
-    return gtNewStoreIndNode(op2->TypeGet(), op1, op2);
+    return gtNewStoreValueNode(op2->TypeGet(), op1, op2);
 }
 
 //----------------------------------------------------------------------------------------------

src/coreclr/jit/simd.cpp

@@ -500,17 +500,4 @@ GenTree* Compiler::impSIMDPopStack()
     return tree;
 }
 
-//-------------------------------------------------------------------
-// Set the flag that indicates that the lclVar referenced by this tree
-// is used in a SIMD intrinsic.
-// Arguments:
-//      tree - GenTree*
-//
-void Compiler::setLclRelatedToSIMDIntrinsic(GenTree* tree)
-{
-    assert(tree->OperIsScalarLocal() || tree->IsLclVarAddr());
-    LclVarDsc* lclVarDsc             = lvaGetDesc(tree->AsLclVarCommon());
-    lclVarDsc->lvUsedInSIMDIntrinsic = true;
-}
-
 #endif // FEATURE_SIMD