[release/10.0] Revert "JIT: Remove side effect detection quirk in loop hoisting (#118463)" with some improvements (#119346)

* Revert "JIT: Remove side effect detection quirk in loop hoisting (#118463)"

This reverts commit e0b14e9. That
change is not correct as it removes the safe guard that prevents
hoisting out of conditionally executed blocks.

* Add test

* Rename m_beforeSideEffect -> m_canHoistSideEffects

* Fix comment

* Move some code around

---------

Co-authored-by: Jakob Botsch Nielsen <[email protected]>

Author: github-actions[bot]

src/coreclr/jit/optimizer.cpp

@@ -3655,12 +3655,21 @@ bool Compiler::optHoistThisLoop(FlowGraphNaturalLoop* loop, LoopHoistContext* ho
     }
 #endif // FEATURE_MASKED_HW_INTRINSICS
 
-    // Find the set of definitely-executed blocks. These will be given priority for hoisting.
+    // Find the set of definitely-executed blocks.
     // Ideally, the definitely-executed blocks are the ones that post-dominate the entry block.
     // Until we have post-dominators, we'll special-case for single-exit blocks.
     //
-    // TODO: We ought to consider hoisting more aggressively from conditionally executed blocks,
-    // if they are frequently executed and it is safe to evaluate the tree early.
+    // Todo: it is not clear if this is a correctness requirement or a profitability heuristic.
+    // It seems like the latter. Ideally there are enough safeguards to prevent hoisting exception
+    // or side-effect dependent things. Note that HoistVisitor uses `m_canHoistSideEffects` to determine if it's
+    // ok to hoist a side-effect. It allows this only for the first block (the entry block), before any
+    // side-effect has been seen. After the first block, it assumes that there has been a side effect and
+    // no further side-effect can be hoisted. It is true that we don't analyze any program behavior in the
+    // flow graph between the entry block and the subsequent blocks, whether they be the next block dominating
+    // the exit block, or the pre-headers of nested loops.
+    //
+    // We really should consider hoisting from conditionally executed blocks, if they are frequently executed
+    // and it is safe to evaluate the tree early.
     //
     assert(m_dfsTree != nullptr);
     BitVecTraits traits(m_dfsTree->PostOrderTraits());
@@ -4116,7 +4125,7 @@ void Compiler::optHoistLoopBlocks(FlowGraphNaturalLoop* loop,
         };
 
         ArrayStack<Value>     m_valueStack;
-        bool                  m_beforeSideEffect;
+        bool                  m_canHoistSideEffects;
         FlowGraphNaturalLoop* m_loop;
         LoopHoistContext*     m_hoistContext;
         BasicBlock*           m_currentBlock;
@@ -4251,7 +4260,7 @@ void Compiler::optHoistLoopBlocks(FlowGraphNaturalLoop* loop,
                      BitVec                defExec)
             : GenTreeVisitor(compiler)
             , m_valueStack(compiler->getAllocator(CMK_LoopHoist))
-            , m_beforeSideEffect(true)
+            , m_canHoistSideEffects(true)
             , m_loop(loop)
             , m_hoistContext(hoistContext)
             , m_currentBlock(nullptr)
@@ -4263,26 +4272,46 @@ void Compiler::optHoistLoopBlocks(FlowGraphNaturalLoop* loop,
         void HoistBlock(BasicBlock* block)
         {
             m_currentBlock = block;
-            for (Statement* const stmt : block->NonPhiStatements())
-            {
-                WalkTree(stmt->GetRootNodePointer(), nullptr);
-                Value& top = m_valueStack.TopRef();
-                assert(top.Node() == stmt->GetRootNode());
 
-                // hoist the top node?
-                if (top.m_hoistable)
-                {
-                    const bool defExecuted = BitVecOps::IsMember(m_traits, m_defExec, block->bbPostorderNum);
-                    m_compiler->optHoistCandidate(stmt->GetRootNode(), block, m_loop, m_hoistContext, defExecuted);
-                }
-                else
+            const weight_t blockWeight = block->getBBWeight(m_compiler);
+
+            JITDUMP("\n    HoistBlock " FMT_BB " (weight=%6s) of loop " FMT_LP " (head: " FMT_BB ")\n", block->bbNum,
+                    refCntWtd2str(blockWeight, /* padForDecimalPlaces */ true), m_loop->GetIndex(),
+                    m_loop->GetHeader()->bbNum);
+
+            if (blockWeight < (BB_UNITY_WEIGHT / 10))
+            {
+                JITDUMP("      block weight is too small to perform hoisting.\n");
+            }
+            else
+            {
+                for (Statement* const stmt : block->NonPhiStatements())
                 {
-                    JITDUMP("      [%06u] %s: %s\n", dspTreeID(top.Node()),
-                            top.m_invariant ? "not hoistable" : "not invariant", top.m_failReason);
-                }
+                    WalkTree(stmt->GetRootNodePointer(), nullptr);
+                    Value& top = m_valueStack.TopRef();
+                    assert(top.Node() == stmt->GetRootNode());
 
-                m_valueStack.Reset();
+                    // hoist the top node?
+                    if (top.m_hoistable)
+                    {
+                        const bool defExecuted = BitVecOps::IsMember(m_traits, m_defExec, block->bbPostorderNum);
+                        m_compiler->optHoistCandidate(stmt->GetRootNode(), block, m_loop, m_hoistContext, defExecuted);
+                    }
+                    else
+                    {
+                        JITDUMP("      [%06u] %s: %s\n", dspTreeID(top.Node()),
+                                top.m_invariant ? "not hoistable" : "not invariant", top.m_failReason);
+                    }
+
+                    m_valueStack.Reset();
+                }
             }
+
+            assert(!m_canHoistSideEffects || (block == m_loop->GetHeader()));
+            // After visiting the first block (which is expected to always be
+            // the loop header) we can no longer hoist out side effecting trees
+            // as the next blocks could be conditionally executed.
+            m_canHoistSideEffects = false;
         }
 
         fgWalkResult PreOrderVisit(GenTree** use, GenTree* user)
@@ -4458,10 +4487,11 @@ void Compiler::optHoistLoopBlocks(FlowGraphNaturalLoop* loop,
 
                 if (treeIsHoistable)
                 {
-                    if (!m_beforeSideEffect)
+                    if (!m_canHoistSideEffects)
                     {
                         // For now, we give up on an expression that might raise an exception if it is after the
-                        // first possible global side effect.
+                        // first possible global side effect (and we assume we're after that if we're not in the first
+                        // block).
                         // TODO-CQ: this is when we might do loop cloning.
                         //
                         if ((tree->gtFlags & GTF_EXCEPT) != 0)
@@ -4484,24 +4514,24 @@ void Compiler::optHoistLoopBlocks(FlowGraphNaturalLoop* loop,
                 }
             }
 
-            // Next check if we need to set 'm_beforeSideEffect' to false.
+            // Next check if we need to set 'm_canHoistSideEffects' to false.
             //
             // If we have already set it to false then we can skip these checks
             //
-            if (m_beforeSideEffect)
+            if (m_canHoistSideEffects)
             {
                 // 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.
+                    // Check if we should clear m_canHoistSideEffects.
+                    // If 'tree' can throw an exception then we need to set m_canHoistSideEffects to false.
                     // Note that calls are handled below
                     if (tree->OperMayThrow(m_compiler) && !tree->IsCall())
                     {
-                        m_beforeSideEffect = false;
+                        m_canHoistSideEffects = false;
                     }
                 }
 
@@ -4517,19 +4547,19 @@ void Compiler::optHoistLoopBlocks(FlowGraphNaturalLoop* loop,
                     GenTreeCall* call = tree->AsCall();
                     if (!call->IsHelperCall())
                     {
-                        m_beforeSideEffect = false;
+                        m_canHoistSideEffects = false;
                     }
                     else
                     {
                         CorInfoHelpFunc helpFunc = eeGetHelperNum(call->gtCallMethHnd);
                         if (s_helperCallProperties.MutatesHeap(helpFunc))
                         {
-                            m_beforeSideEffect = false;
+                            m_canHoistSideEffects = false;
                         }
                         else if (s_helperCallProperties.MayRunCctor(helpFunc) &&
                                  (call->gtFlags & GTF_CALL_HOISTABLE) == 0)
                         {
-                            m_beforeSideEffect = false;
+                            m_canHoistSideEffects = false;
                         }
 
                         // Additional check for helper calls that throw exceptions
@@ -4541,7 +4571,7 @@ void Compiler::optHoistLoopBlocks(FlowGraphNaturalLoop* loop,
                             // Does this helper call throw?
                             if (!s_helperCallProperties.NoThrow(helpFunc))
                             {
-                                m_beforeSideEffect = false;
+                                m_canHoistSideEffects = false;
                             }
                         }
                     }
@@ -4562,8 +4592,8 @@ void Compiler::optHoistLoopBlocks(FlowGraphNaturalLoop* loop,
                     if (isGloballyVisibleStore)
                     {
                         INDEBUG(failReason = "store to globally visible memory");
-                        treeIsHoistable    = false;
-                        m_beforeSideEffect = false;
+                        treeIsHoistable       = false;
+                        m_canHoistSideEffects = false;
                     }
                 }
             }
@@ -4667,22 +4697,8 @@ void Compiler::optHoistLoopBlocks(FlowGraphNaturalLoop* loop,
     };
 
     HoistVisitor visitor(this, loop, hoistContext, traits, defExecuted);
-    loop->VisitLoopBlocks([&](BasicBlock* block) -> BasicBlockVisit {
-        const weight_t blockWeight = block->getBBWeight(this);
-
-        JITDUMP("\n    optHoistLoopBlocks " FMT_BB " (weight=%6s) of loop " FMT_LP " (head: " FMT_BB ")\n",
-                block->bbNum, refCntWtd2str(blockWeight, /* padForDecimalPlaces */ true), loop->GetIndex(),
-                loop->GetHeader()->bbNum);
-
-        if (blockWeight < (BB_UNITY_WEIGHT / 10))
-        {
-            JITDUMP("      block weight is too small to perform hoisting.\n");
-        }
-        else
-        {
-            visitor.HoistBlock(block);
-        }
-
+    loop->VisitLoopBlocksReversePostOrder([&](BasicBlock* block) -> BasicBlockVisit {
+        visitor.HoistBlock(block);
         return BasicBlockVisit::Continue;
     });
 

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

@@ -0,0 +1,60 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Numerics;
+using System.Runtime.Intrinsics;
+using Xunit;
+
+public class Runtime_119061
+{
+    private static C1 s_6 = new C1();
+    
+    [Fact]
+    public static void TestEntryPoint()
+    {
+        s_6.F8 = new S0(-1);
+        var vr4 = s_6.F8.F0;
+        Vector128<short> vr6 = default(Vector128<short>);
+        M3(vr4, vr6);
+    }
+
+    private static ushort M3(int arg1, Vector128<short> arg2)
+    {
+        bool[] var0 = new bool[]
+        {
+            false
+        };
+        for (sbyte lvar1 = 10; lvar1 < 12; lvar1++)
+        {
+            if (var0[0])
+            {
+                var vr2 = (uint)(3329109910U / (-9223372036854775808L / (arg1 | 1)));
+                var vr1 = (short)BitOperations.LeadingZeroCount(vr2);
+                arg2 = Vector128.CreateScalar(vr1);
+            }
+        }
+
+        C0 vr8 = new C0();
+        return vr8.F1;
+    }
+
+    private struct S0
+    {
+        public int F0;
+        public S0(int f0) : this()
+        {
+            F0 = f0;
+        }
+    }
+
+    private class C0
+    {
+        public ushort F1;
+    }
+
+    private class C1
+    {
+        public S0 F8;
+    }
+}

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

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