Fix issue #26417 = Incorrect caching of loop variable

Contributes to issue  #7147 - JIT: Loop hoisting re-ordering exceptions
Added the Test case for Issue 26417

src/jit/optimizer.cpp

@@ -6795,8 +6795,20 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack<BasicBlock*>* blo
 
         bool IsTreeVNInvariant(GenTree* tree)
         {
-            return m_compiler->optVNIsLoopInvariant(tree->gtVNPair.GetLiberal(), m_loopNum,
-                                                    &m_hoistContext->m_curLoopVnInvariantCache);
+            ValueNum vn = tree->gtVNPair.GetLiberal();
+            if (m_compiler->vnStore->IsVNConstant(vn))
+            {
+                // It is unsafe to allow a GT_CLS_VAR that has been assigned a constant.
+                // The logic in optVNIsLoopInvariant would consider it to be loop-invariant, even
+                // if the assignment of the constant to the GT_CLS_VAR was inside the loop.
+                //
+                if (tree->OperIs(GT_CLS_VAR))
+                {
+                    return false;
+                }
+            }
+
+            return m_compiler->optVNIsLoopInvariant(vn, m_loopNum, &m_hoistContext->m_curLoopVnInvariantCache);
         }
 
     public:
@@ -7015,41 +7027,55 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack<BasicBlock*>* blo
             //
             if (m_beforeSideEffect)
             {
-                // For this purpose, we only care about memory side effects.  We assume that expressions will
-                // be hoisted so that they are evaluated in the same order as they would have been in the loop,
-                // and therefore throw exceptions in the same order.  (So we don't use GTF_GLOBALLY_VISIBLE_SIDE_EFFECTS
-                // here, since that includes exceptions.)
-                if (tree->IsCall())
+                // If the current tree isn't invariant (and thus prevents hoisting) then we have to check for any possible side effect
+                if (!treeIsInvariant)
                 {
-                    // If it's a call, it must be a helper call that does not mutate the heap.
-                    // Further, if it may run a cctor, it must be labeled as "Hoistable"
-                    // (meaning it won't run a cctor because the class is not precise-init).
-                    GenTreeCall* call = tree->AsCall();
-                    if (call->gtCallType != CT_HELPER)
+                    // Here we know that we have a tree that we are not goimg to hoist
+                    // If it has any side effects, then we need to set m_beforeSideEffect to false.
+                    if ((tree->gtFlags & GTF_EXCEPT) != 0)
                     {
                         m_beforeSideEffect = false;
                     }
-                    else
+                }
+                else // this tree is invariant and could still be hoisted 
+                {
+                    // For this purpose, we only care about memory side effects.  We assume that expressions will
+                    // be hoisted so that they are evaluated in the same order as they would have been in the loop,
+                    // and therefore throw exceptions in the same order.  (So we don't use
+                    // GTF_GLOBALLY_VISIBLE_SIDE_EFFECTS
+                    // here, since that includes exceptions.)
+                    if (tree->IsCall())
                     {
-                        CorInfoHelpFunc helpFunc = eeGetHelperNum(call->gtCallMethHnd);
-                        if (s_helperCallProperties.MutatesHeap(helpFunc))
+                        // If it's a call, it must be a helper call that does not mutate the heap.
+                        // Further, if it may run a cctor, it must be labeled as "Hoistable"
+                        // (meaning it won't run a cctor because the class is not precise-init).
+                        GenTreeCall* call = tree->AsCall();
+                        if (call->gtCallType != CT_HELPER)
                         {
                             m_beforeSideEffect = false;
                         }
-                        else if (s_helperCallProperties.MayRunCctor(helpFunc) &&
-                                 (call->gtFlags & GTF_CALL_HOISTABLE) == 0)
+                        else
                         {
-                            m_beforeSideEffect = false;
+                            CorInfoHelpFunc helpFunc = eeGetHelperNum(call->gtCallMethHnd);
+                            if (s_helperCallProperties.MutatesHeap(helpFunc))
+                            {
+                                m_beforeSideEffect = false;
+                            }
+                            else if (s_helperCallProperties.MayRunCctor(helpFunc) &&
+                                     (call->gtFlags & GTF_CALL_HOISTABLE) == 0)
+                            {
+                                m_beforeSideEffect = false;
+                            }
                         }
                     }
-                }
-                else if (tree->OperIs(GT_ASG))
-                {
-                    // If the LHS of the assignment has a global reference, then assume it's a global side effect.
-                    GenTree* lhs = tree->gtOp.gtOp1;
-                    if (lhs->gtFlags & GTF_GLOB_REF)
+                    else if (tree->OperIs(GT_ASG))
                     {
-                        m_beforeSideEffect = false;
+                        // If the LHS of the assignment has a global reference, then assume it's a global side effect.
+                        GenTree* lhs = tree->gtOp.gtOp1;
+                        if (lhs->gtFlags & GTF_GLOB_REF)
+                        {
+                            m_beforeSideEffect = false;
+                        }
                     }
                 }
             }

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

@@ -0,0 +1,47 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using System.Runtime.CompilerServices;
+
+class GitHub_26417
+{
+    static int   _a;
+
+    [MethodImplAttribute(MethodImplOptions.NoInlining)]
+    static void MyWriteLine(int v)
+    {
+        Console.WriteLine(v);
+        if (v == 0)
+        {
+            throw new Exception();
+        }
+    }
+
+    [MethodImplAttribute(MethodImplOptions.NoInlining)]
+    static void Test()
+    {
+        _a = 1;
+
+        while (_a == 1)
+        {
+            MyWriteLine(_a);
+            _a = 0;
+        }
+    }
+
+    static int Main()
+    {
+        int result = 100;
+        try {
+            Test();
+        }
+        catch (Exception)
+        {
+            Console.WriteLine("FAILED");
+            result = -1;
+        }
+        return result;
+    }
+}

tests/src/JIT/Regression/JitBlue/GitHub_26417/GitHub_26417.csproj

@@ -0,0 +1,12 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>