Add ValueNumbering support for GT_SIMD and GT_HWINTRINSIC tree nodes (#31834)

* Added ValueNumbering support for GT_SIMD and GT_HWINTRINSIC tree nodes

* Allow SIMD and HW Intrinsics to be CSE candidates

* Correctness fix for optAssertionPropMain
  - Zero out the bbAssertionIn values, as these can be referenced in RangeCheck::MergeAssertion
    and this is shared state with the CSE phase: bbCseIn

* Improve the VNFOA_ArityMask

* Update to use the new TARGET macros

* Include node type when value numbering SIMDIntrinsicInit
Mutate the gloabl heap when performing a HW_INTRINSIC memory store operation
Printing of SIMD constants only support 0

* Disable CSE's for some special HW_INTRINSIC categories

* Code review feedback

* Record csdStructHnd; // The class handle, currently needed to create a SIMD LclVar in PerformCSE

* Instead of asserting on a struct handle mismatch, we record it in csdStructHndMismatch and avoid making the candidate into a CSE

* Fix the JITDUMP messages to print the CseIndex

* add check for (newElemStructHnd != NO_CLASS_HANDLE)

* Additional checks for SIMD struct types when setting csdStructHnd
Added Mismatched Struct Handle assert in ConsiderCandidates

* fix GenTreeSIMD::OperIsMemoryLoad for ARM64
Removed ismatched Struct Handle assert

* Fix the printing of BitSets on Linux, change the printf format specifier

* Added check for simdNode->OperIsMemoryLoad()) to fgValueNumberSimd
Added bool OperIsMemoryLoad() to GenTreeSIMD, returns true for SIMDIntrinsicInitArray
Added valuenumfuncs.h to src/coreclr/src/jit/CMakeLists.txt

* Avoid calling gtGetStructHandleIfPresent to set csdStructHnd when we have a GT_IND node

* Fix check for (newElemStructHnd != hashDsc->csdStructHnd)

* added extra value number argument VNF_SimdType for Most SIMD operations
added VNF_SimdType // A value number function to compose a SIMD type
added vnDumpSimdType

* Added bool methods vnEncodesResultTypeForSIMDIntrinsic and vnEncodesResultTypeForHWIntrinsic
these return true when a SIMD or HW Instrinsic will use an extra arg to record the result type during value numbering
Changes the ValueNumFuncDef to set the arity to zero when a -1 value is passed in
Updated InitValueNumStoreStatics to fixup the arity of SIMD or HW Instrinsic that have an extra arg to record the result type
Allow a type mismatchj when we have a GT_BLK as the lhs of an assignment, as it is used to zero out Simd structs

* Fix for SIMD_WidenLo arg count

* fix typo

* Fix x86 build breaks
Fix SIMD_WidenHi

* Added method header comment for vnEncodesResultTypeForHWIntrinsic
Added & VNFOA_ArityMask when assigning to vnfOpAttribs[]

* Codereview feedback and some more comments

* fix typo

* Moved the code that sets the arg count for the three SIMD intrinsics

* clang-format

* Adjust CSE for SIMD types that are live across a call

* Proposed fix for #32085

* Revert "Proposed fix for #32085"

This reverts commit 169c24e.

* Added better comments for optcse SIMD caller saved register heuristics

* Added CONFIG_INTEGER: JitDisableSimdVN,
   Default 0, ValueNumbering of SIMD nodes and HW Intrinsic nodes enabled
   If 1, then disable ValueNumbering of SIMD nodes
   If 2, then disable ValueNumbering of HW Intrinsic nodes
   If 3, disable both SIMD and HW Intrinsic nodes

* Moved JitDisableSimdVN from DEBUG to RETAIL

Author: briansull

src/coreclr/src/jit/CMakeLists.txt

@@ -186,6 +186,7 @@ if (CLR_CMAKE_TARGET_WIN32)
     unwind.h
     utils.h
     valuenum.h
+    valuenumfuncs.h
     valuenumtype.h
     varset.h
     vartype.h

src/coreclr/src/jit/assertionprop.cpp

@@ -5193,8 +5193,15 @@ void Compiler::optAssertionPropMain()
         }
     }
 
-    if (!optAssertionCount)
+    if (optAssertionCount == 0)
     {
+        // Zero out the bbAssertionIn values, as these can be referenced in RangeCheck::MergeAssertion
+        // and this is sharedstate with the CSE phase: bbCseIn
+        //
+        for (BasicBlock* block = fgFirstBB; block; block = block->bbNext)
+        {
+            block->bbAssertionIn = BitVecOps::MakeEmpty(apTraits);
+        }
         return;
     }
 

src/coreclr/src/jit/bitsetasshortlong.h

@@ -412,12 +412,12 @@ class BitSetOps</*BitSetType*/ BitSetShortLongRep,
             char*    ptr             = res;
             if (sizeof(size_t) == sizeof(int64_t))
             {
-                sprintf_s(ptr, remaining, "%016zX", bits);
+                sprintf_s(ptr, remaining, "%016llX", bits);
             }
             else
             {
                 assert(sizeof(size_t) == sizeof(int));
-                sprintf_s(ptr, remaining, "%08zX", bits);
+                sprintf_s(ptr, remaining, "%08X", bits);
             }
             return res;
         }

src/coreclr/src/jit/compiler.h

@@ -4674,6 +4674,16 @@ class Compiler
     // Does value-numbering for an intrinsic tree.
     void fgValueNumberIntrinsic(GenTree* tree);
 
+#ifdef FEATURE_SIMD
+    // Does value-numbering for a GT_SIMD tree
+    void fgValueNumberSimd(GenTree* tree);
+#endif // FEATURE_SIMD
+
+#ifdef FEATURE_HW_INTRINSICS
+    // Does value-numbering for a GT_HWINTRINSIC tree
+    void fgValueNumberHWIntrinsic(GenTree* tree);
+#endif // FEATURE_HW_INTRINSICS
+
     // Does value-numbering for a call.  We interpret some helper calls.
     void fgValueNumberCall(GenTreeCall* call);
 
@@ -4698,6 +4708,9 @@ class Compiler
     // Adds the exception set for the current tree node which is performing a overflow checking operation
     void fgValueNumberAddExceptionSetForOverflow(GenTree* tree);
 
+    // Adds the exception set for the current tree node which is performing a bounds check operation
+    void fgValueNumberAddExceptionSetForBoundsCheck(GenTree* tree);
+
     // Adds the exception set for the current tree node which is performing a ckfinite operation
     void fgValueNumberAddExceptionSetForCkFinite(GenTree* tree);
 
@@ -6190,6 +6203,12 @@ class Compiler
         treeStmtLst* csdTreeList; // list of matching tree nodes: head
         treeStmtLst* csdTreeLast; // list of matching tree nodes: tail
 
+        // ToDo: This can be removed when gtGetStructHandleIfPresent stops guessing
+        // and GT_IND nodes always have valid struct handle.
+        //
+        CORINFO_CLASS_HANDLE csdStructHnd; // The class handle, currently needed to create a SIMD LclVar in PerformCSE
+        bool                 csdStructHndMismatch;
+
         ValueNum defExcSetPromise; // The exception set that is now required for all defs of this CSE.
                                    // This will be set to NoVN if we decide to abandon this CSE
 
@@ -8189,6 +8208,13 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 #endif // !FEATURE_SIMD
     }
 
+#ifdef FEATURE_SIMD
+    static bool vnEncodesResultTypeForSIMDIntrinsic(SIMDIntrinsicID intrinsicId);
+#endif // !FEATURE_SIMD
+#ifdef FEATURE_HW_INTRINSICS
+    static bool vnEncodesResultTypeForHWIntrinsic(NamedIntrinsic hwIntrinsicID);
+#endif // FEATURE_HW_INTRINSICS
+
 private:
     // These routines need not be enclosed under FEATURE_SIMD since lvIsSIMDType()
     // is defined for both FEATURE_SIMD and !FEATURE_SIMD apropriately. The use

src/coreclr/src/jit/gentree.cpp

@@ -5405,11 +5405,16 @@ bool GenTree::OperIsImplicitIndir() const
         case GT_ARR_ELEM:
         case GT_ARR_OFFSET:
             return true;
+#ifdef FEATURE_SIMD
+        case GT_SIMD:
+        {
+            return AsSIMD()->OperIsMemoryLoad();
+        }
+#endif // FEATURE_SIMD
 #ifdef FEATURE_HW_INTRINSICS
         case GT_HWINTRINSIC:
         {
-            GenTreeHWIntrinsic* hwIntrinsicNode = (const_cast<GenTree*>(this))->AsHWIntrinsic();
-            return hwIntrinsicNode->OperIsMemoryLoadOrStore();
+            return AsHWIntrinsic()->OperIsMemoryLoadOrStore();
         }
 #endif // FEATURE_HW_INTRINSICS
         default:
@@ -18224,6 +18229,16 @@ bool GenTree::isCommutativeSIMDIntrinsic()
             return false;
     }
 }
+
+// Returns true for the SIMD Instrinsic instructions that have MemoryLoad semantics, false otherwise
+bool GenTreeSIMD::OperIsMemoryLoad() const
+{
+    if (gtSIMDIntrinsicID == SIMDIntrinsicInitArray)
+    {
+        return true;
+    }
+    return false;
+}
 #endif // FEATURE_SIMD
 
 #ifdef FEATURE_HW_INTRINSICS
@@ -18433,7 +18448,7 @@ GenTree* Compiler::gtNewMustThrowException(unsigned helper, var_types type, CORI
 }
 
 // Returns true for the HW Instrinsic instructions that have MemoryLoad semantics, false otherwise
-bool GenTreeHWIntrinsic::OperIsMemoryLoad()
+bool GenTreeHWIntrinsic::OperIsMemoryLoad() const
 {
 #ifdef TARGET_XARCH
     // Some xarch instructions have MemoryLoad sematics
@@ -18475,7 +18490,7 @@ bool GenTreeHWIntrinsic::OperIsMemoryLoad()
 }
 
 // Returns true for the HW Instrinsic instructions that have MemoryStore semantics, false otherwise
-bool GenTreeHWIntrinsic::OperIsMemoryStore()
+bool GenTreeHWIntrinsic::OperIsMemoryStore() const
 {
 #ifdef TARGET_XARCH
     // Some xarch instructions have MemoryStore sematics
@@ -18510,7 +18525,7 @@ bool GenTreeHWIntrinsic::OperIsMemoryStore()
 }
 
 // Returns true for the HW Instrinsic instructions that have MemoryLoad semantics, false otherwise
-bool GenTreeHWIntrinsic::OperIsMemoryLoadOrStore()
+bool GenTreeHWIntrinsic::OperIsMemoryLoadOrStore() const
 {
 #ifdef TARGET_XARCH
     return OperIsMemoryLoad() || OperIsMemoryStore();

src/coreclr/src/jit/gentree.h

@@ -4536,6 +4536,9 @@ struct GenTreeSIMD : public GenTreeJitIntrinsic
     {
     }
 
+    bool OperIsMemoryLoad() const; // Returns true for the SIMD Instrinsic instructions that have MemoryLoad semantics,
+                                   // false otherwise
+
 #if DEBUGGABLE_GENTREE
     GenTreeSIMD() : GenTreeJitIntrinsic()
     {
@@ -4584,12 +4587,12 @@ struct GenTreeHWIntrinsic : public GenTreeJitIntrinsic
     // 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,
+    bool OperIsMemoryLoad() const;  // 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,
+    bool OperIsMemoryStore() const; // 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
+    bool OperIsMemoryLoadOrStore() const; // Returns true for the HW Instrinsic instructions that have MemoryLoad or
+                                          // MemoryStore semantics, false otherwise
 
 #if DEBUGGABLE_GENTREE
     GenTreeHWIntrinsic() : GenTreeJitIntrinsic()

src/coreclr/src/jit/hwintrinsic.cpp

@@ -173,6 +173,69 @@ CORINFO_CLASS_HANDLE Compiler::gtGetStructHandleForHWSIMD(var_types simdType, va
     return NO_CLASS_HANDLE;
 }
 
+#ifdef FEATURE_HW_INTRINSICS
+//------------------------------------------------------------------------
+// vnEncodesResultTypeForHWIntrinsic(NamedIntrinsic hwIntrinsicID):
+//
+// Arguments:
+//    hwIntrinsicID -- The id for the HW intrinsic
+//
+// Return Value:
+//   Returns true if this intrinsic requires value numbering to add an
+//   extra SimdType argument that encodes the resulting type.
+//   If we don't do this overloaded versions can return the same VN
+//   leading to incorrect CSE subsitutions.
+//
+/* static */ bool Compiler::vnEncodesResultTypeForHWIntrinsic(NamedIntrinsic hwIntrinsicID)
+{
+    int numArgs = HWIntrinsicInfo::lookupNumArgs(hwIntrinsicID);
+
+    // HW Instrinsic's with -1 for numArgs have a varying number of args, so we currently
+    // give themm a unique value number them, and don't add an extra argument.
+    //
+    if (numArgs == -1)
+    {
+        return false;
+    }
+
+    // We iterate over all of the different baseType's for this instrinsic in the HWIntrinsicInfo table
+    // We set  diffInsCount to the number of instructions that can execute differently.
+    //
+    unsigned diffInsCount = 0;
+#ifdef TARGET_XARCH
+    instruction lastIns = INS_invalid;
+#endif
+    for (var_types baseType = TYP_BYTE; (baseType <= TYP_DOUBLE); baseType = (var_types)(baseType + 1))
+    {
+        instruction curIns = HWIntrinsicInfo::lookupIns(hwIntrinsicID, baseType);
+        if (curIns != INS_invalid)
+        {
+#ifdef TARGET_XARCH
+            if (curIns != lastIns)
+            {
+                diffInsCount++;
+                // remember the last valid instruction that we saw
+                lastIns = curIns;
+            }
+#elif defined(TARGET_ARM64)
+            // On ARM64 we use the same instruction and specify an insOpt arrangement
+            // so we always consider the instruction operation to be different
+            //
+            diffInsCount++;
+#endif // TARGET
+            if (diffInsCount >= 2)
+            {
+                // We can  early exit the loop now
+                break;
+            }
+        }
+    }
+
+    // If we see two (or more) different instructions we need the extra VNF_SimdType arg
+    return (diffInsCount >= 2);
+}
+#endif // FEATURE_HW_INTRINSICS
+
 //------------------------------------------------------------------------
 // lookupId: Gets the NamedIntrinsic for a given method name and InstructionSet
 //

src/coreclr/src/jit/hwintrinsiclistxarch.h

@@ -483,10 +483,10 @@ HARDWARE_INTRINSIC(AVX2_ConvertToUInt32,                            "ConvertToUI
 HARDWARE_INTRINSIC(AVX2_ConvertToVector256Int16,                    "ConvertToVector256Int16",                      AVX2,         -1,              32,           1,     {INS_pmovsxbw,          INS_pmovzxbw,       INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid},           HW_Category_SimpleSIMD,             HW_Flag_SpecialCodeGen|HW_Flag_BaseTypeFromFirstArg)
 HARDWARE_INTRINSIC(AVX2_ConvertToVector256Int32,                    "ConvertToVector256Int32",                      AVX2,         -1,              32,           1,     {INS_pmovsxbd,          INS_pmovzxbd,       INS_pmovsxwd,       INS_pmovzxwd,       INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid},           HW_Category_SimpleSIMD,             HW_Flag_SpecialCodeGen|HW_Flag_BaseTypeFromFirstArg)
 HARDWARE_INTRINSIC(AVX2_ConvertToVector256Int64,                    "ConvertToVector256Int64",                      AVX2,         -1,              32,           1,     {INS_pmovsxbq,          INS_pmovzxbq,       INS_pmovsxwq,       INS_pmovzxwq,       INS_pmovsxdq,       INS_pmovzxdq,       INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid},           HW_Category_SimpleSIMD,             HW_Flag_SpecialCodeGen|HW_Flag_BaseTypeFromFirstArg)
-HARDWARE_INTRINSIC(AVX2_GatherVector128,                            "GatherVector128",                              AVX2,         -1,              16,            3,     {INS_invalid,           INS_invalid,        INS_invalid,       INS_invalid,        INS_vpgatherdd,     INS_vpgatherdd,     INS_vpgatherdq,     INS_vpgatherdq,     INS_vgatherdps,     INS_vgatherdpd},        HW_Category_IMM,                    HW_Flag_SpecialCodeGen|HW_Flag_NoContainment)
-HARDWARE_INTRINSIC(AVX2_GatherVector256,                            "GatherVector256",                              AVX2,         -1,              32,            3,     {INS_invalid,           INS_invalid,        INS_invalid,       INS_invalid,        INS_vpgatherdd,     INS_vpgatherdd,     INS_vpgatherdq,     INS_vpgatherdq,     INS_vgatherdps,     INS_vgatherdpd},        HW_Category_IMM,                    HW_Flag_MaybeMemoryLoad|HW_Flag_SpecialCodeGen|HW_Flag_NoContainment)
-HARDWARE_INTRINSIC(AVX2_GatherMaskVector128,                        "GatherMaskVector128",                          AVX2,         -1,              16,            5,     {INS_invalid,           INS_invalid,        INS_invalid,       INS_invalid,        INS_vpgatherdd,     INS_vpgatherdd,     INS_vpgatherdq,     INS_vpgatherdq,     INS_vgatherdps,     INS_vgatherdpd},        HW_Category_IMM,                    HW_Flag_MaybeMemoryLoad|HW_Flag_SpecialCodeGen|HW_Flag_SpecialImport|HW_Flag_NoContainment)
-HARDWARE_INTRINSIC(AVX2_GatherMaskVector256,                        "GatherMaskVector256",                          AVX2,         -1,              32,            5,     {INS_invalid,           INS_invalid,        INS_invalid,       INS_invalid,        INS_vpgatherdd,     INS_vpgatherdd,     INS_vpgatherdq,     INS_vpgatherdq,     INS_vgatherdps,     INS_vgatherdpd},        HW_Category_IMM,                    HW_Flag_MaybeMemoryLoad|HW_Flag_SpecialCodeGen|HW_Flag_SpecialImport|HW_Flag_NoContainment)
+HARDWARE_INTRINSIC(AVX2_GatherVector128,                            "GatherVector128",                              AVX2,         -1,              16,           3,     {INS_invalid,           INS_invalid,        INS_invalid,        INS_invalid,        INS_vpgatherdd,     INS_vpgatherdd,     INS_vpgatherdq,     INS_vpgatherdq,     INS_vgatherdps,     INS_vgatherdpd},        HW_Category_IMM,                    HW_Flag_SpecialCodeGen|HW_Flag_NoContainment)
+HARDWARE_INTRINSIC(AVX2_GatherVector256,                            "GatherVector256",                              AVX2,         -1,              32,           3,     {INS_invalid,           INS_invalid,        INS_invalid,        INS_invalid,        INS_vpgatherdd,     INS_vpgatherdd,     INS_vpgatherdq,     INS_vpgatherdq,     INS_vgatherdps,     INS_vgatherdpd},        HW_Category_IMM,                    HW_Flag_MaybeMemoryLoad|HW_Flag_SpecialCodeGen|HW_Flag_NoContainment)
+HARDWARE_INTRINSIC(AVX2_GatherMaskVector128,                        "GatherMaskVector128",                          AVX2,         -1,              16,           5,     {INS_invalid,           INS_invalid,        INS_invalid,        INS_invalid,        INS_vpgatherdd,     INS_vpgatherdd,     INS_vpgatherdq,     INS_vpgatherdq,     INS_vgatherdps,     INS_vgatherdpd},        HW_Category_IMM,                    HW_Flag_MaybeMemoryLoad|HW_Flag_SpecialCodeGen|HW_Flag_SpecialImport|HW_Flag_NoContainment)
+HARDWARE_INTRINSIC(AVX2_GatherMaskVector256,                        "GatherMaskVector256",                          AVX2,         -1,              32,           5,     {INS_invalid,           INS_invalid,        INS_invalid,        INS_invalid,        INS_vpgatherdd,     INS_vpgatherdd,     INS_vpgatherdq,     INS_vpgatherdq,     INS_vgatherdps,     INS_vgatherdpd},        HW_Category_IMM,                    HW_Flag_MaybeMemoryLoad|HW_Flag_SpecialCodeGen|HW_Flag_SpecialImport|HW_Flag_NoContainment)
 HARDWARE_INTRINSIC(AVX2_HorizontalAdd,                              "HorizontalAdd",                                AVX2,         -1,              32,           2,     {INS_invalid,           INS_invalid,        INS_phaddw,         INS_invalid,        INS_phaddd,         INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid},           HW_Category_SimpleSIMD,             HW_Flag_NoFlag)
 HARDWARE_INTRINSIC(AVX2_HorizontalAddSaturate,                      "HorizontalAddSaturate",                        AVX2,         -1,              32,           2,     {INS_invalid,           INS_invalid,        INS_phaddsw,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid},           HW_Category_SimpleSIMD,             HW_Flag_NoFlag)
 HARDWARE_INTRINSIC(AVX2_HorizontalSubtract,                         "HorizontalSubtract",                           AVX2,         -1,              32,           2,     {INS_invalid,           INS_invalid,        INS_phsubw,         INS_invalid,        INS_phsubd,         INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid,        INS_invalid},           HW_Category_SimpleSIMD,             HW_Flag_NoFlag)

src/coreclr/src/jit/jitconfigvalues.h

@@ -135,6 +135,7 @@ CONFIG_INTEGER(ShouldInjectFault, W("InjectFault"), 0)
 CONFIG_INTEGER(StressCOMCall, W("StressCOMCall"), 0)
 CONFIG_INTEGER(TailcallStress, W("TailcallStress"), 0)
 CONFIG_INTEGER(TreesBeforeAfterMorph, W("JitDumpBeforeAfterMorph"), 0) // If 1, display each tree before/after morphing
+
 CONFIG_METHODSET(JitBreak, W("JitBreak")) // Stops in the importer when compiling a specified method
 CONFIG_METHODSET(JitDebugBreak, W("JitDebugBreak"))
 CONFIG_METHODSET(JitDisasm, W("JitDisasm"))                  // Dumps disassembly for specified method
@@ -275,6 +276,14 @@ CONFIG_INTEGER(EnableArm64Sve,          W("EnableArm64Sve"), 1)
 
 // clang-format on
 
+#ifdef FEATURE_SIMD
+CONFIG_INTEGER(JitDisableSimdVN, W("JitDisableSimdVN"), 0) // Default 0, ValueNumbering of SIMD nodes and HW Intrinsic
+                                                           // nodes enabled
+                                                           // If 1, then disable ValueNumbering of SIMD nodes
+                                                           // If 2, then disable ValueNumbering of HW Intrinsic nodes
+                                                           // If 3, disable both SIMD and HW Intrinsic nodes
+#endif                                                     // FEATURE_SIMD
+
 ///
 /// JIT
 ///