Arm: Include BBJ_ALWAYS blocks in dominance calculation (#59376)

* Include BBJ_ALWAYS in inverse post ordering for computing reachability

* Add test case

* Add BBJ_ALWAYS in fgAlways list

* jit format

src/coreclr/jit/compiler.h

@@ -4979,6 +4979,12 @@ class Compiler
 
     BlockSet fgEnterBlks; // Set of blocks which have a special transfer of control; the "entry" blocks plus EH handler
                           // begin blocks.
+#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
+    BlockSet fgAlwaysBlks; // Set of blocks which are BBJ_ALWAYS part  of BBJ_CALLFINALLY/BBJ_ALWAYS pair that should
+                           // never be removed due to a requirement to use the BBJ_ALWAYS for generating code and
+                           // not have "retless" blocks.
+
+#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
 
 #ifdef DEBUG
     bool fgReachabilitySetsValid; // Are the bbReach sets valid?

src/coreclr/jit/fgbasic.cpp

@@ -119,6 +119,10 @@ void Compiler::fgInit()
     /* This is set by fgComputeReachability */
     fgEnterBlks = BlockSetOps::UninitVal();
 
+#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
+    fgAlwaysBlks = BlockSetOps::UninitVal();
+#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
+
 #ifdef DEBUG
     fgEnterBlksSetValid = false;
 #endif // DEBUG

src/coreclr/jit/fgopt.cpp

@@ -297,6 +297,10 @@ void Compiler::fgComputeEnterBlocksSet()
 
     fgEnterBlks = BlockSetOps::MakeEmpty(this);
 
+#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
+    fgAlwaysBlks = BlockSetOps::MakeEmpty(this);
+#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
+
     /* Now set the entry basic block */
     BlockSetOps::AddElemD(this, fgEnterBlks, fgFirstBB->bbNum);
     assert(fgFirstBB->bbNum == 1);
@@ -315,19 +319,15 @@ void Compiler::fgComputeEnterBlocksSet()
     }
 
 #if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
-    // TODO-ARM-Cleanup: The ARM code here to prevent creating retless calls by adding the BBJ_ALWAYS
-    // to the enter blocks is a bit of a compromise, because sometimes the blocks are already reachable,
-    // and it messes up DFS ordering to have them marked as enter block. We should prevent the
-    // creation of retless calls some other way.
+    // For ARM code, prevent creating retless calls by adding the BBJ_ALWAYS to the "fgAlwaysBlks" list.
     for (BasicBlock* const block : Blocks())
     {
         if (block->bbJumpKind == BBJ_CALLFINALLY)
         {
             assert(block->isBBCallAlwaysPair());
 
-            // Don't remove the BBJ_ALWAYS block that is only here for the unwinder. It might be dead
-            // if the finally is no-return, so mark it as an entry point.
-            BlockSetOps::AddElemD(this, fgEnterBlks, block->bbNext->bbNum);
+            // Don't remove the BBJ_ALWAYS block that is only here for the unwinder.
+            BlockSetOps::AddElemD(this, fgAlwaysBlks, block->bbNext->bbNum);
         }
     }
 #endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
@@ -401,6 +401,13 @@ bool Compiler::fgRemoveUnreachableBlocks()
             {
                 goto SKIP_BLOCK;
             }
+
+#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
+            if (!BlockSetOps::IsEmptyIntersection(this, fgAlwaysBlks, block->bbReach))
+            {
+                goto SKIP_BLOCK;
+            }
+#endif // defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
         }
 
         // Remove all the code for the block
@@ -636,23 +643,7 @@ void Compiler::fgDfsInvPostOrder()
     // an incoming edge into the block).
     assert(fgEnterBlksSetValid);
 
-#if defined(FEATURE_EH_FUNCLETS) && defined(TARGET_ARM)
-    //
-    //    BlockSetOps::UnionD(this, startNodes, fgEnterBlks);
-    //
-    // This causes problems on ARM, because we for BBJ_CALLFINALLY/BBJ_ALWAYS pairs, we add the BBJ_ALWAYS
-    // to the enter blocks set to prevent flow graph optimizations from removing it and creating retless call finallies
-    // (BBF_RETLESS_CALL). This leads to an incorrect DFS ordering in some cases, because we start the recursive walk
-    // from the BBJ_ALWAYS, which is reachable from other blocks. A better solution would be to change ARM to avoid
-    // creating retless calls in a different way, not by adding BBJ_ALWAYS to fgEnterBlks.
-    //
-    // So, let us make sure at least fgFirstBB is still there, even if it participates in a loop.
-    BlockSetOps::AddElemD(this, startNodes, 1);
-    assert(fgFirstBB->bbNum == 1);
-#else
     BlockSetOps::UnionD(this, startNodes, fgEnterBlks);
-#endif
-
     assert(BlockSetOps::IsMember(this, startNodes, fgFirstBB->bbNum));
 
     // Call the flowgraph DFS traversal helper.

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

@@ -0,0 +1,66 @@
+using System.Runtime.CompilerServices;
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+//
+// Note: In below test case, we do not iterate over BBJ_ALWAYS blocks while computing the
+//       reachability leading to assert
+public class Runtime_59298
+{
+    public struct S2
+    {
+        public struct S2_D1_F1
+        {
+            public double double_1;
+        }
+    }
+    static int s_int_6 = -2;
+    static S2 s_s2_16 = new S2();
+    int int_6 = -2;
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public int LeafMethod6()
+    {
+        return 1;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public S2 Method0(out short p_short_1)
+    {
+        long long_7 = -1;
+        p_short_1 = 0;
+        switch (long_7)
+        {
+            case -5:
+                {
+                    do
+                    {
+                        try
+                        {
+                            int_6 ^= int_6;
+                        }
+                        finally
+                        {
+                            // The expression doesn't matter, it just has to be long enough
+                            // to have few extra blocks which we don't walk when doing inverse
+                            // post order while computing dominance information.
+                            long_7 &= long_7;
+                            int_6 &= (int_6 /= (int_6 -= LeafMethod6() - int_6) + 69) / ((int_6 << (int_6 - int_6)) + (int_6 |= LeafMethod6()) + (LeafMethod6() >> s_int_6) + 62);
+                        }
+                    }
+                    while (long_7 == 8);
+                    break;
+                }
+            default:
+                {
+                    break;
+                }
+        }
+        return s_s2_16;
+    }
+
+    public static int Main(string[] args)
+    {
+        new Runtime_59298().Method0(out short s);
+        return s + 100;
+    }
+}

src/tests/JIT/Regression/JitBlue/Runtime_59298/Runtime_59298.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>