Potential fix for folding of *(typ*)&lclVar for small types #40607 (#…

…40871)

* Add regression test for #40607

* Add Runtime_40607.tt

* Add more extensive tests for loads in Runtime_40607.tt Runtime_40607.il

* Enable back failing test in System.Private.Xml.dll

* Fold *(typ*)&lclVar tree when:

1) it is *definitely load* and types of both indirection and local variable have the same signedness (e.g. bool and byte)
2) otherwise, fold the tree and mark the local node with GTF_VAR_FOLDED_IND
   and call fgDoNormalizeOnStore() on such nodes' parents in post-order morph.

src/coreclr/src/jit/compiler.h

@@ -476,8 +476,6 @@ class LclVarDsc
     unsigned char lvContainsHoles : 1;     // True when we have a promoted struct that contains holes
     unsigned char lvCustomLayout : 1;      // True when this struct has "CustomLayout"
 
-    unsigned char lvForceLoadNormalize : 1; // True when this local had a cast on the LHS of an assignment
-
     unsigned char lvIsMultiRegArg : 1; // true if this is a multireg LclVar struct used in an argument context
     unsigned char lvIsMultiRegRet : 1; // true if this is a multireg LclVar struct assigned from a multireg call
 
@@ -886,14 +884,14 @@ class LclVarDsc
     {
         return varTypeIsSmall(TypeGet()) &&
                // lvIsStructField is treated the same as the aliased local, see fgDoNormalizeOnStore.
-               (lvIsParam || lvAddrExposed || lvIsStructField || lvForceLoadNormalize);
+               (lvIsParam || lvAddrExposed || lvIsStructField);
     }
 
     bool lvNormalizeOnStore() const
     {
         return varTypeIsSmall(TypeGet()) &&
                // lvIsStructField is treated the same as the aliased local, see fgDoNormalizeOnStore.
-               !(lvIsParam || lvAddrExposed || lvIsStructField || lvForceLoadNormalize);
+               !(lvIsParam || lvAddrExposed || lvIsStructField);
     }
 
     void incRefCnts(BasicBlock::weight_t weight,

src/coreclr/src/jit/flowgraph.cpp

@@ -21623,6 +21623,10 @@ void Compiler::fgDebugCheckFlags(GenTree* tree)
                 chkFlags |= GTF_GLOB_REF | GTF_ASG;
                 break;
 
+            case GT_LCL_VAR:
+                assert((tree->gtFlags & GTF_VAR_FOLDED_IND) == 0);
+                break;
+
             default:
                 break;
         }

src/coreclr/src/jit/gentree.h

@@ -815,6 +815,9 @@ struct GenTree
 #define GTF_VAR_ITERATOR    0x00800000 // GT_LCL_VAR -- this is a iterator reference in the loop condition
 #define GTF_VAR_CLONED      0x00400000 // GT_LCL_VAR -- this node has been cloned or is a clone
 #define GTF_VAR_CONTEXT     0x00200000 // GT_LCL_VAR -- this node is part of a runtime lookup
+#define GTF_VAR_FOLDED_IND  0x00100000 // GT_LCL_VAR -- this node was folded from *(typ*)&lclVar expression tree in fgMorphSmpOp()
+// where 'typ' is a small type and 'lclVar' corresponds to a normalized-on-store local variable.
+// This flag identifies such nodes in order to make sure that fgDoNormalizeOnStore() is called on their parents in post-order morph.
 
                                        // Relevant for inlining optimizations (see fgInlinePrependStatements)
 

src/coreclr/src/jit/morph.cpp

@@ -12657,6 +12657,13 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
     {
         case GT_ASG:
 
+            if (op1->OperIs(GT_LCL_VAR) && ((op1->gtFlags & GTF_VAR_FOLDED_IND) != 0))
+            {
+                op1->gtFlags &= ~GTF_VAR_FOLDED_IND;
+                tree = fgDoNormalizeOnStore(tree);
+                op2  = tree->gtGetOp2();
+            }
+
             lclVarTree = fgIsIndirOfAddrOfLocal(op1);
             if (lclVarTree != nullptr)
             {
@@ -13615,17 +13622,16 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
             break;
 
         case GT_IND:
-
+        {
             // Can not remove a GT_IND if it is currently a CSE candidate.
             if (gtIsActiveCSE_Candidate(tree))
             {
                 break;
             }
 
-            bool foldAndReturnTemp;
-            foldAndReturnTemp = false;
-            temp              = nullptr;
-            ival1             = 0;
+            bool foldAndReturnTemp = false;
+            temp                   = nullptr;
+            ival1                  = 0;
 
             // Don't remove a volatile GT_IND, even if the address points to a local variable.
             if ((tree->gtFlags & GTF_IND_VOLATILE) == 0)
@@ -13655,11 +13661,6 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
                         unsigned   lclNum = temp->AsLclVarCommon()->GetLclNum();
                         LclVarDsc* varDsc = &lvaTable[lclNum];
 
-                        // Note that fgMorph uses GTF_DONT_CSE to mark the left side of an assignment
-                        // Thus stores have this flag and load do not have this flag
-                        //
-                        bool isLoad = (tree->gtFlags & GTF_DONT_CSE) == 0;
-
                         // We will try to optimize when we have a promoted struct promoted with a zero lvFldOffset
                         if (varDsc->lvPromoted && (varDsc->lvFldOffset == 0))
                         {
@@ -13689,25 +13690,25 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
                         // correctly normalized.
                         //
                         // The below transformation cannot be applied if the local var needs to be normalized on load.
-                        else if (varTypeIsSmall(typ) && (genTypeSize(lvaTable[lclNum].lvType) == genTypeSize(typ)) &&
+                        else if (varTypeIsSmall(typ) && (genTypeSize(varDsc) == genTypeSize(typ)) &&
                                  !lvaTable[lclNum].lvNormalizeOnLoad())
                         {
-                            // For any stores of small types, we will force loads to be normalized
-                            // this is necessary as we need to zero/sign extend any load
-                            // after this kind of store.
-                            //
-                            if (!isLoad)
-                            {
-                                varDsc->lvForceLoadNormalize = true;
-                            }
-                            // otherwise we have a load operation
-                            //
-                            // And for loads signed/unsigned differences do matter.
-                            //
-                            else if (varTypeIsUnsigned(lvaTable[lclNum].lvType) == varTypeIsUnsigned(typ))
+                            const bool definitelyLoad = (tree->gtFlags & GTF_DONT_CSE) == 0;
+                            const bool possiblyStore  = !definitelyLoad;
+
+                            if (possiblyStore || (varTypeIsUnsigned(varDsc) == varTypeIsUnsigned(typ)))
                             {
-                                tree->gtType = typ = temp->TypeGet();
-                                foldAndReturnTemp  = true;
+                                typ               = temp->TypeGet();
+                                tree->gtType      = typ;
+                                foldAndReturnTemp = true;
+
+                                if (possiblyStore)
+                                {
+                                    // This node can be on the left-hand-side of an assignment node.
+                                    // Mark this node with GTF_VAR_FOLDED_IND to make sure that fgDoNormalizeOnStore()
+                                    // is called on its parent in post-order morph.
+                                    temp->gtFlags |= GTF_VAR_FOLDED_IND;
+                                }
                             }
                         }
                         // For matching types we can fold
@@ -13984,6 +13985,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
             }
 
             break;
+        }
 
         case GT_ADDR:
 

src/libraries/System.Private.Xml/tests/Xslt/XslTransformApi/CXslTransformMultith.cs

@@ -76,7 +76,6 @@ public virtual int Transform(object args)
         //[Variation("Local and global variables", Params = new object[] { "xslt_mutith_variable_local_and_global.xsl", "xslt_mutith_variable_local_and_global.xsl" })]
         [InlineData("xslt_mutith_variable_local_and_global.xsl", "xslt_mutith_variable_local_and_global.xsl")]
         [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
-        [SkipOnCoreClr("https://github.com/dotnet/runtime/issues/40607", RuntimeTestModes.JitStress)]
         public void Variations(object param0, object param1)
         {
             string xslFile = (string)param0;

src/tests/JIT/Regression/JitBlue/Runtime_40607/Runtime_40607.cs

@@ -0,0 +1,69 @@
+using System;
+using System.Runtime.CompilerServices;
+
+namespace Runtime_40607
+{
+    class Program
+    {
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        static bool WillBeInlined(out bool shouldBeFalse)
+        {
+            shouldBeFalse = false;
+            return true;
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        [SkipLocalsInit]
+        static int DependsOnUnInitValue()
+        {
+            int retVal = 1;
+            bool shouldBeFalse;
+
+            while (WillBeInlined(out shouldBeFalse))
+            {
+                if (shouldBeFalse)
+                {
+                    retVal = 0;
+                }
+                break;
+            }
+
+            return retVal;
+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        static unsafe int PoisonStackWith(uint fillValue)
+        {
+            int retVal = 1;
+            bool shouldBeFalse;
+
+            *(uint*)&shouldBeFalse = fillValue;
+
+            while (WillBeInlined(out shouldBeFalse))
+            {
+                if (shouldBeFalse)
+                {
+                    retVal = 0;
+                }
+                break;
+            }
+
+            return retVal;
+        }
+
+        static int Main(string[] args)
+        {
+            PoisonStackWith(0xdeadbeef);
+
+            const int expected = 1;
+            int actual = DependsOnUnInitValue();
+
+            if (expected != actual)
+            {
+                return 0;
+            }
+
+            return 100;
+        }
+    }
+}