Apply redundant branch optimizations in post-order of dominators (#57074)

In #56979 we end up with outdated dominators and make a wrong decision
based on it. To avoid this situation, visit basic blocks in post-order
of the dominator tree to ensure that we have up-to-date dominators for
each basic block.

Fix #56979

diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp
index b52193f..55568c3 100644 (file)
--- a/src/coreclr/jit/redundantbranchopts.cpp
+++ b/src/coreclr/jit/redundantbranchopts.cpp
@@ -19,24 +19,40 @@ PhaseStatus Compiler::optRedundantBranches()
     }
 #endif // DEBUG
 
-    bool madeChanges = false;
-
-    for (BasicBlock* const block : Blocks())
+    class OptRedundantBranchesDomTreeVisitor : public DomTreeVisitor<OptRedundantBranchesDomTreeVisitor>
     {
-        // Skip over any removed blocks.
-        //
-        if ((block->bbFlags & BBF_REMOVED) != 0)
+    public:
+        bool madeChanges;
+
+        OptRedundantBranchesDomTreeVisitor(Compiler* compiler)
+            : DomTreeVisitor(compiler, compiler->fgSsaDomTree), madeChanges(false)
         {
-            continue;
         }
 
-        // We currently can optimize some BBJ_CONDs.
-        //
-        if (block->bbJumpKind == BBJ_COND)
+        void PreOrderVisit(BasicBlock* block)
         {
-            madeChanges |= optRedundantBranch(block);
         }
-    }
+
+        void PostOrderVisit(BasicBlock* block)
+        {
+            // Skip over any removed blocks.
+            //
+            if ((block->bbFlags & BBF_REMOVED) != 0)
+            {
+                return;
+            }
+
+            // We currently can optimize some BBJ_CONDs.
+            //
+            if (block->bbJumpKind == BBJ_COND)
+            {
+                madeChanges |= m_compiler->optRedundantBranch(block);
+            }
+        }
+    };
+
+    OptRedundantBranchesDomTreeVisitor visitor(this);
+    visitor.WalkTree();
 
     // Reset visited flags, in case we set any.
     //
@@ -46,13 +62,13 @@ PhaseStatus Compiler::optRedundantBranches()
     }
 
 #if DEBUG
-    if (verbose && madeChanges)
+    if (verbose && visitor.madeChanges)
     {
         fgDispBasicBlocks(verboseTrees);
     }
 #endif // DEBUG
 
-    return madeChanges ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING;
+    return visitor.madeChanges ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING;
 }
 
 //------------------------------------------------------------------------

diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_57074/Runtime_57074.cs b/src/tests/JIT/Regression/JitBlue/Runtime_57074/Runtime_57074.cs
new file mode 100644 (file)
index 0000000..e400265
--- /dev/null
+++ b/src/tests/JIT/Regression/JitBlue/Runtime_57074/Runtime_57074.cs
@@ -0,0 +1,76 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+// Generated by Fuzzlyn v1.2 on 2021-08-06 13:43:59
+// Run on .NET 6.0.0-dev on X64 Windows
+// Seed: 4503995795470420928
+// Reduced from 174.4 KiB to 1.0 KiB in 00:01:45
+// Debug: Outputs 0
+// Release: Outputs 1
+using System.Runtime.CompilerServices;
+
+public class Program
+{
+    static bool s_5 = true;
+    static uint[] s_8;
+    static uint s_9 = 1;
+    static long s_13;
+    static uint s_15;
+    public static int Main()
+    {
+        s_5 = s_5; // Make sure we get no static helpers in function below
+        M49();
+        return s_9 == 0 ? 100 : -1;
+    }
+
+    // Redundant branch opts would optimize BB03 -> BB05 to go BB03 -> BB06.
+    // After this, the dominator of BB06 changes to BB02, but when we processed
+    // it we still saw BB05. This caused us to follow up by changing BB03 -> BB08.
+    static void M49()
+    {
+        for (int var0 = 0; var0 < Bound(); var0++)
+        {
+            // BB02
+            long var1 = 0;
+            if (s_5)
+            {
+                // BB03
+                var1 = s_13;
+            }
+            else
+            {
+                // BB04
+                s_8[0] = 0;
+            }
+
+            // BB05
+            if (s_5)
+            {
+                // BB06
+                if (s_5)
+                {
+                    // BB07
+                    s_9 = 0;
+                }
+
+                // BB08
+                long var2 = var1;
+                return;
+            }
+            else
+            {
+                try
+                {
+                    s_8[0] = s_9;
+                }
+                finally
+                {
+                    var1 = 0;
+                }
+            }
+        }
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static int Bound() => 1;
+}

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