New fix for dotnet/coreclr#26417 - Don't allow the hoisting of GT_CLS_VARs that were...

* Fix issue dotnet/coreclr#26417 = Incorrect caching of loop variable
Contributes to issue  dotnet/coreclr#7147 - JIT: Loop hoisting re-ordering exceptions
Added the Test case for Issue 26417

Commit migrated from https://github.com/dotnet/coreclr/commit/da6edcd43fb83d6843208a82f719e49984b7e59e

diff --git a/src/coreclr/src/jit/optimizer.cpp b/src/coreclr/src/jit/optimizer.cpp
index 7f3bb2b..5c56442 100644 (file)
--- a/src/coreclr/src/jit/optimizer.cpp
+++ b/src/coreclr/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:
@@ -7003,22 +7015,36 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack<BasicBlock*>* blo
                 // Is the value of the whole tree loop invariant?
                 if (!treeIsInvariant)
                 {
+                    // Here we have a tree that is not loop invariant and we thus cannot hoist
                     treeIsHoistable = false;
                 }
             }
 
-            // Check if we need to set 'm_beforeSideEffect' to false.
-            // If we encounter a tree with a call in it
-            //  or if we see an assignment to global we set it to false.
+            // Next check if we need to set 'm_beforeSideEffect' to false.
             //
-            // If we are already set to false then we can skip these checks
+            // If we have already set it to false then we can skip these checks
             //
             if (m_beforeSideEffect)
             {
-                // For this purpose, we only care about memory side effects.  We assume that expressions will
+                // Is the value of the whole tree loop invariant?
+                if (!treeIsInvariant)
+                {
+                    // We have a tree that is not loop invariant and we thus cannot hoist
+                    assert(treeIsHoistable == false);
+
+                    // Check if we should clear m_beforeSideEffect.
+                    // If 'tree' can throw an exception then we need to set m_beforeSideEffect to false.
+                    // Note that calls are handled below
+                    if (tree->OperMayThrow(m_compiler) && !tree->IsCall())
+                    {
+                        m_beforeSideEffect = false;
+                    }
+                }
+
+                // In the section below, 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.)
+                // and therefore throw exceptions in the same order.
+                //
                 if (tree->IsCall())
                 {
                     // If it's a call, it must be a helper call that does not mutate the heap.
@@ -7041,6 +7067,19 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack<BasicBlock*>* blo
                         {
                             m_beforeSideEffect = false;
                         }
+
+                        // Additional check for helper calls that throw exceptions
+                        if (!treeIsInvariant)
+                        {
+                            // We have a tree that is not loop invariant and we thus cannot hoist
+                            assert(treeIsHoistable == false);
+
+                            // Does this helper call throw?
+                            if (!s_helperCallProperties.NoThrow(helpFunc))
+                            {
+                                m_beforeSideEffect = false;
+                            }
+                        }
                     }
                 }
                 else if (tree->OperIs(GT_ASG))
@@ -7052,6 +7091,13 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack<BasicBlock*>* blo
                         m_beforeSideEffect = false;
                     }
                 }
+                else if (tree->OperIs(GT_XADD, GT_XCHG, GT_LOCKADD, GT_CMPXCHG, GT_MEMORYBARRIER))
+                {
+                    // If this node is a MEMORYBARRIER or an Atomic operation
+                    // then don't hoist and stop any further hoisting after this node
+                    treeIsHoistable    = false;
+                    m_beforeSideEffect = false;
+                }
             }
 
             // If this 'tree' is hoistable then we return and the caller will

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_26417/GitHub_26417.cs b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_26417/GitHub_26417.cs
new file mode 100644 (file)
index 0000000..6871962
--- /dev/null
+++ b/src/coreclr/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;
+    }
+}

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_26417/GitHub_26417.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_26417/GitHub_26417.csproj
new file mode 100644 (file)
index 0000000..f3e1cbd
--- /dev/null
+++ b/src/coreclr/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>