Mutate the global heap valuenumber for any HW intrinsic that performs a memory store operation

Use fgMutateGcHeap to record memory write operations by HW Intrinsics
Set flags for the HW Intrinsic nodes that access Memory
Added support for HWIntrinsic nodes to OperMayThrow
Added support for GT_HWIntrinsic to GenTree::OperRequiresAsgFlag() and GenTree::OperIsImplicitIndir()
Refactored GenTreeHWIntrinsic::OperIsMemoryLoad() and GenTreeHWIntrinsic::OperIsMemoryStore()
Added GenTreeHWIntrinsic::OperIsMemoryLoadOrStore()
Deleted the static version of OperIsImplicitIndir(gtOper)

6 files changed, 254 insertions, 34 deletions

diff --git a/src/jit/compiler.h b/src/jit/compiler.h
index a39f714d9e..ebaf58e3c6 100644
--- a/src/jit/compiler.h
+++ b/src/jit/compiler.h
@@ -3100,10 +3100,14 @@ protected:
     bool isScalarISA(InstructionSet isa);
     static int ivalOfHWIntrinsic(NamedIntrinsic intrinsic);
     unsigned simdSizeOfHWIntrinsic(NamedIntrinsic intrinsic, CORINFO_SIG_INFO* sig);
-    static int numArgsOfHWIntrinsic(GenTreeHWIntrinsic* node);
     static GenTree* lastOpOfHWIntrinsic(GenTreeHWIntrinsic* node, int numArgs);
     static instruction insOfHWIntrinsic(NamedIntrinsic intrinsic, var_types type);
+
+public:
     static HWIntrinsicCategory categoryOfHWIntrinsic(NamedIntrinsic intrinsic);
+    static int numArgsOfHWIntrinsic(GenTreeHWIntrinsic* node);
+
+protected:
     static HWIntrinsicFlag flagsOfHWIntrinsic(NamedIntrinsic intrinsic);
     GenTree* getArgForHWIntrinsic(var_types argType, CORINFO_CLASS_HANDLE argClass);
     static int immUpperBoundOfHWIntrinsic(NamedIntrinsic intrinsic);
diff --git a/src/jit/gentree.cpp b/src/jit/gentree.cpp
index 4614df0e37..3846b2ea7e 100644
--- a/src/jit/gentree.cpp
+++ b/src/jit/gentree.cpp
@@ -2118,6 +2118,7 @@ AGAIN:
                 case GT_SIMD:
                     hash += tree->gtSIMD.gtSIMDIntrinsicID;
                     hash += tree->gtSIMD.gtSIMDBaseType;
+                    hash += tree->gtSIMD.gtSIMDSize;
                     break;
 #endif // FEATURE_SIMD
 
@@ -2125,6 +2126,7 @@ AGAIN:
                 case GT_HWIntrinsic:
                     hash += tree->gtHWIntrinsic.gtHWIntrinsicId;
                     hash += tree->gtHWIntrinsic.gtSIMDBaseType;
+                    hash += tree->gtHWIntrinsic.gtSIMDSize;
                     break;
 #endif // FEATURE_HW_INTRINSICS
 
@@ -5992,8 +5994,66 @@ GenTree* GenTree::gtGetParent(GenTree*** parentChildPtrPtr) const
 
 bool GenTree::OperRequiresAsgFlag()
 {
-    return (OperIsAssignment() || (gtOper == GT_XADD) || (gtOper == GT_XCHG) || (gtOper == GT_LOCKADD) ||
-            (gtOper == GT_CMPXCHG) || (gtOper == GT_MEMORYBARRIER));
+    if (OperIsAssignment() || OperIs(GT_XADD, GT_XCHG, GT_LOCKADD, GT_CMPXCHG, GT_MEMORYBARRIER))
+    {
+        return true;
+    }
+#ifdef FEATURE_HW_INTRINSICS
+    if (gtOper == GT_HWIntrinsic)
+    {
+        GenTreeHWIntrinsic* hwIntrinsicNode = this->AsHWIntrinsic();
+        if (hwIntrinsicNode->OperIsMemoryStore())
+        {
+            // A MemoryStore operation is an assignment
+            return true;
+        }
+    }
+#endif // FEATURE_HW_INTRINSICS
+    return false;
+}
+
+//------------------------------------------------------------------------------
+// OperIsImplicitIndir : Check whether the operation contains an implicit
+//                       indirection.
+// Arguments:
+//    this      -  a GenTree node
+//
+// Return Value:
+//    True if the given node contains an implicit indirection
+//
+// Note that for the GT_HWIntrinsic node we have to examine the
+// details of the node to determine its result.
+//
+
+bool GenTree::OperIsImplicitIndir() const
+{
+    switch (gtOper)
+    {
+        case GT_LOCKADD:
+        case GT_XADD:
+        case GT_XCHG:
+        case GT_CMPXCHG:
+        case GT_BLK:
+        case GT_OBJ:
+        case GT_DYN_BLK:
+        case GT_STORE_BLK:
+        case GT_STORE_OBJ:
+        case GT_STORE_DYN_BLK:
+        case GT_BOX:
+        case GT_ARR_INDEX:
+        case GT_ARR_ELEM:
+        case GT_ARR_OFFSET:
+            return true;
+#ifdef FEATURE_HW_INTRINSICS
+        case GT_HWIntrinsic:
+        {
+            GenTreeHWIntrinsic* hwIntrinsicNode = (const_cast<GenTree*>(this))->AsHWIntrinsic();
+            return hwIntrinsicNode->OperIsMemoryLoadOrStore();
+        }
+#endif // FEATURE_HW_INTRINSICS
+        default:
+            return false;
+    }
 }
 
 //------------------------------------------------------------------------------
@@ -6079,6 +6139,22 @@ bool GenTree::OperMayThrow(Compiler* comp)
 #endif // FEATURE_HW_INTRINSICS
         case GT_INDEX_ADDR:
             return true;
+
+#ifdef FEATURE_HW_INTRINSICS
+        case GT_HWIntrinsic:
+        {
+            GenTreeHWIntrinsic* hwIntrinsicNode = this->AsHWIntrinsic();
+            assert(hwIntrinsicNode != nullptr);
+            if (hwIntrinsicNode->OperIsMemoryStore() || hwIntrinsicNode->OperIsMemoryLoad())
+            {
+                // This operation contains an implicit indirection
+                //   it could throw a null reference exception.
+                //
+                return true;
+            }
+        }
+#endif // FEATURE_HW_INTRINSICS
+
         default:
             break;
     }
@@ -18151,6 +18227,114 @@ GenTree* Compiler::gtNewMustThrowException(unsigned helper, var_types type, CORI
     }
     return node;
 }
+
+// Returns true for the HW Instrinsic instructions that have MemoryLoad semantics, false otherwise
+bool GenTreeHWIntrinsic::OperIsMemoryLoad()
+{
+#ifdef _TARGET_XARCH_
+    // Some xarch instructions have MemoryLoad sematics
+    HWIntrinsicCategory category = Compiler::categoryOfHWIntrinsic(gtHWIntrinsicId);
+    if (category == HW_Category_MemoryLoad)
+    {
+        return true;
+    }
+    else if (category == HW_Category_IMM)
+    {
+        // Some AVX instructions here also have MemoryLoad sematics
+
+        // Do we have 3 operands?
+        if (Compiler::numArgsOfHWIntrinsic(this) != 3)
+        {
+            return false;
+        }
+        else // We have 3 operands/args
+        {
+            GenTreeArgList* argList = gtOp.gtOp1->AsArgList();
+
+            if ((gtHWIntrinsicId == NI_AVX_InsertVector128 || gtHWIntrinsicId == NI_AVX2_InsertVector128) &&
+                (argList->Current()->TypeGet() == TYP_I_IMPL)) // Is the type of the first arg TYP_I_IMPL?
+            {
+                // This is Avx/Avx2.InsertVector128
+                return true;
+            }
+        }
+    }
+#endif // _TARGET_XARCH_
+    return false;
+}
+
+// Returns true for the HW Instrinsic instructions that have MemoryStore semantics, false otherwise
+bool GenTreeHWIntrinsic::OperIsMemoryStore()
+{
+#ifdef _TARGET_XARCH_
+    // Some xarch instructions have MemoryStore sematics
+    HWIntrinsicCategory category = Compiler::categoryOfHWIntrinsic(gtHWIntrinsicId);
+    if (category == HW_Category_MemoryStore)
+    {
+        return true;
+    }
+    else if (category == HW_Category_IMM)
+    {
+        // Some AVX instructions here also have MemoryStore sematics
+
+        // Do we have 3 operands?
+        if (Compiler::numArgsOfHWIntrinsic(this) != 3)
+        {
+            return false;
+        }
+        else // We have 3 operands/args
+        {
+            if ((gtHWIntrinsicId == NI_AVX_ExtractVector128 || gtHWIntrinsicId == NI_AVX2_ExtractVector128))
+            {
+                // This is Avx/Avx2.ExtractVector128
+                return true;
+            }
+        }
+    }
+#endif // _TARGET_XARCH_
+    return false;
+}
+
+// Returns true for the HW Instrinsic instructions that have MemoryLoad semantics, false otherwise
+bool GenTreeHWIntrinsic::OperIsMemoryLoadOrStore()
+{
+#ifdef _TARGET_XARCH_
+    // Some xarch instructions have MemoryLoad sematics
+    HWIntrinsicCategory category = Compiler::categoryOfHWIntrinsic(gtHWIntrinsicId);
+    if ((category == HW_Category_MemoryLoad) || (category == HW_Category_MemoryStore))
+    {
+        return true;
+    }
+    else if (category == HW_Category_IMM)
+    {
+        // Some AVX instructions here also have MemoryLoad or MemoryStore sematics
+
+        // Do we have 3 operands?
+        if (Compiler::numArgsOfHWIntrinsic(this) != 3)
+        {
+            return false;
+        }
+        else // We have 3 operands/args
+        {
+            GenTreeArgList* argList = gtOp.gtOp1->AsArgList();
+
+            if ((gtHWIntrinsicId == NI_AVX_InsertVector128 || gtHWIntrinsicId == NI_AVX2_InsertVector128) &&
+                (argList->Current()->TypeGet() == TYP_I_IMPL)) // Is the type of the first arg TYP_I_IMPL?
+            {
+                // This is Avx/Avx2.InsertVector128
+                return true;
+            }
+            else if ((gtHWIntrinsicId == NI_AVX_ExtractVector128 || gtHWIntrinsicId == NI_AVX2_ExtractVector128))
+            {
+                // This is Avx/Avx2.ExtractVector128
+                return true;
+            }
+        }
+    }
+#endif // _TARGET_XARCH_
+    return false;
+}
+
 #endif // FEATURE_HW_INTRINSICS
 
 //---------------------------------------------------------------------------------------
diff --git a/src/jit/gentree.h b/src/jit/gentree.h
index 4ed7cf0461..8b9198fd13 100644
--- a/src/jit/gentree.h
+++ b/src/jit/gentree.h
@@ -1553,34 +1553,7 @@ public:
         return OperIsIndirOrArrLength(gtOper);
     }
 
-    static bool OperIsImplicitIndir(genTreeOps gtOper)
-    {
-        switch (gtOper)
-        {
-            case GT_LOCKADD:
-            case GT_XADD:
-            case GT_XCHG:
-            case GT_CMPXCHG:
-            case GT_BLK:
-            case GT_OBJ:
-            case GT_DYN_BLK:
-            case GT_STORE_BLK:
-            case GT_STORE_OBJ:
-            case GT_STORE_DYN_BLK:
-            case GT_BOX:
-            case GT_ARR_INDEX:
-            case GT_ARR_ELEM:
-            case GT_ARR_OFFSET:
-                return true;
-            default:
-                return false;
-        }
-    }
-
-    bool OperIsImplicitIndir() const
-    {
-        return OperIsImplicitIndir(gtOper);
-    }
+    bool OperIsImplicitIndir() const;
 
     bool OperIsStore() const
     {
@@ -4288,6 +4261,17 @@ struct GenTreeHWIntrinsic : public GenTreeJitIntrinsic
     {
     }
 
+    // Note that HW Instrinsic instructions are a sub class of GenTreeOp which only supports two operands
+    // However there are HW Instrinsic instructions that have 3 or even 4 operands and this is
+    // supported using a single op1 and using an ArgList for it:  gtNewArgList(op1, op2, op3)
+
+    bool OperIsMemoryLoad();        // Returns true for the HW Instrinsic instructions that have MemoryLoad semantics,
+                                    // false otherwise
+    bool OperIsMemoryStore();       // Returns true for the HW Instrinsic instructions that have MemoryStore semantics,
+                                    // false otherwise
+    bool OperIsMemoryLoadOrStore(); // Returns true for the HW Instrinsic instructions that have MemoryLoad or
+                                    // MemoryStore semantics, false otherwise
+
 #if DEBUGGABLE_GENTREE
     GenTreeHWIntrinsic() : GenTreeJitIntrinsic()
     {
diff --git a/src/jit/hwintrinsicxarch.cpp b/src/jit/hwintrinsicxarch.cpp
index 9edf5facf1..002a22026c 100644
--- a/src/jit/hwintrinsicxarch.cpp
+++ b/src/jit/hwintrinsicxarch.cpp
@@ -816,9 +816,9 @@ GenTree* Compiler::impHWIntrinsic(NamedIntrinsic        intrinsic,
         assert(insOfHWIntrinsic(intrinsic, baseType) != INS_invalid);
         assert(simdSize == 32 || simdSize == 16);
 
-        GenTree* retNode = nullptr;
-        GenTree* op1     = nullptr;
-        GenTree* op2     = nullptr;
+        GenTreeHWIntrinsic* retNode = nullptr;
+        GenTree*            op1     = nullptr;
+        GenTree*            op2     = nullptr;
 
         switch (numArgs)
         {
@@ -865,6 +865,22 @@ GenTree* Compiler::impHWIntrinsic(NamedIntrinsic        intrinsic,
             default:
                 unreached();
         }
+
+        bool isMemoryStore = retNode->OperIsMemoryStore();
+        if (isMemoryStore || retNode->OperIsMemoryLoad())
+        {
+            if (isMemoryStore)
+            {
+                // A MemoryStore operation is an assignment
+                retNode->gtFlags |= GTF_ASG;
+            }
+
+            // This operation contains an implicit indirection
+            //   it could point into the gloabal heap or
+            //   it could throw a null reference exception.
+            //
+            retNode->gtFlags |= (GTF_GLOB_REF | GTF_EXCEPT);
+        }
         return retNode;
     }
 
diff --git a/src/jit/liveness.cpp b/src/jit/liveness.cpp
index 7ae9487ecd..a4ef055782 100644
--- a/src/jit/liveness.cpp
+++ b/src/jit/liveness.cpp
@@ -329,6 +329,27 @@ void Compiler::fgPerNodeLocalVarLiveness(GenTree* tree)
             fgCurMemoryDef |= memoryKindSet(GcHeap, ByrefExposed);
             break;
 
+#ifdef FEATURE_HW_INTRINSICS
+        case GT_HWIntrinsic:
+        {
+            GenTreeHWIntrinsic* hwIntrinsicNode = tree->AsHWIntrinsic();
+
+            // We can't call fgMutateGcHeap unless the block has recorded a MemoryDef
+            //
+            if (hwIntrinsicNode->OperIsMemoryStore())
+            {
+                // We currently handle this like a Volatile store, so it counts as a definition of GcHeap/ByrefExposed
+                fgCurMemoryDef |= memoryKindSet(GcHeap, ByrefExposed);
+            }
+            if (hwIntrinsicNode->OperIsMemoryLoad())
+            {
+                // This instruction loads from memory and we need to record this information
+                fgCurMemoryUse |= memoryKindSet(GcHeap, ByrefExposed);
+            }
+            break;
+        }
+#endif
+
         // For now, all calls read/write GcHeap/ByrefExposed, writes in their entirety.  Might tighten this case later.
         case GT_CALL:
         {
diff --git a/src/jit/valuenum.cpp b/src/jit/valuenum.cpp
index 15a970d725..5b8537e599 100644
--- a/src/jit/valuenum.cpp
+++ b/src/jit/valuenum.cpp
@@ -5632,6 +5632,17 @@ void Compiler::fgValueNumberTree(GenTree* tree, bool evalAsgLhsInd)
     {
         // TODO-CQ: For now hardware intrinsics are not handled by value numbering to be amenable for CSE'ing.
         tree->gtVNPair.SetBoth(vnStore->VNForExpr(compCurBB, TYP_UNKNOWN));
+
+        GenTreeHWIntrinsic* hwIntrinsicNode = tree->AsHWIntrinsic();
+        assert(hwIntrinsicNode != nullptr);
+
+        // For safety/correctness we must mutate the global heap valuenumber
+        //  for any HW intrinsic that performs a memory store operation
+        if (hwIntrinsicNode->OperIsMemoryStore())
+        {
+            fgMutateGcHeap(tree DEBUGARG("HWIntrinsic - MemoryStore"));
+        }
+
         return;
     }
 #endif // FEATURE_HW_INTRINSICS