From 925e26ed9332759de970b4df281e11d4c01d6ee0 Mon Sep 17 00:00:00 2001
From: Andy Ayers <andya@microsoft.com>
Date: Tue, 21 Jun 2022 12:39:42 -0700
Subject: [PATCH] JIT: avoid cloning mid-entry loops with multiple non-loop
 entry preds (#70959)

If a mid-entry loop has several non-loop preds, some of them may be edges from
enclosing loop constructs that were not recognized as loops on their own.

Avoid cloning such loops. We won't do proper invariance anlysis as we are not
properly recognizing the extent of the loop as is, and we won't get the flow
connnected up properly post cloning (so that the fast loop is proper loop where
the entry is dominated by the head).

This is a workaround for a fairly rare case. Such loops are never iterable and
so we will only try cloning when these loops have an invariant type test.

Ideally we would extend loop canonicalization to cover this case, essentially
ensuring that all recognized loops have a preheader -- that is, that the entry
has just one non-loop predecessor. Currently we  do this only for top entry
loops. But doing that with our current setup looked more complex and we don't
expect to see many of these cases.

Closes #70802.
---
 src/coreclr/jit/loopcloning.cpp               | 19 ++++++
 src/tests/JIT/opt/Cloning/Runtime_70802.cs    | 64 +++++++++++++++++++
 .../JIT/opt/Cloning/Runtime_70802.csproj      | 19 ++++++
 3 files changed, 102 insertions(+)
 create mode 100644 src/tests/JIT/opt/Cloning/Runtime_70802.cs
 create mode 100644 src/tests/JIT/opt/Cloning/Runtime_70802.csproj

diff --git a/src/coreclr/jit/loopcloning.cpp b/src/coreclr/jit/loopcloning.cpp
index 915f19525ff..05c96837bc4 100644
--- a/src/coreclr/jit/loopcloning.cpp
+++ b/src/coreclr/jit/loopcloning.cpp
@@ -1724,6 +1724,25 @@ bool Compiler::optIsLoopClonable(unsigned loopInd)
         return false;
     }
 
+    // Reject cloning if this is a mid-entry loop and the entry has non-loop predecessors other than its head.
+    // This loop may be part of a larger looping construct that we didn't recognize.
+    //
+    // We should really fix this in optCanonicalizeLoop.
+    //
+    if (!loop.lpIsTopEntry())
+    {
+        for (BasicBlock* const entryPred : loop.lpEntry->PredBlocks())
+        {
+            if ((entryPred != loop.lpHead) && !loop.lpContains(entryPred))
+            {
+                JITDUMP("Loop cloning: rejecting loop " FMT_LP
+                        ". Is not top entry, and entry has multiple non-loop preds.\n",
+                        loopInd);
+                return false;
+            }
+        }
+    }
+
     // We've previously made a decision whether to have separate return epilogs, or branch to one.
     // There's a GCInfo limitation in the x86 case, so that there can be no more than SET_EPILOGCNT_MAX separate
     // epilogs.  Other architectures have a limit of 4 here for "historical reasons", but this should be revisited
diff --git a/src/tests/JIT/opt/Cloning/Runtime_70802.cs b/src/tests/JIT/opt/Cloning/Runtime_70802.cs
new file mode 100644
index 00000000000..d10c799d257
--- /dev/null
+++ b/src/tests/JIT/opt/Cloning/Runtime_70802.cs
@@ -0,0 +1,64 @@
+// 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.Runtime.CompilerServices;
+using System.Threading;
+
+public class B 
+{
+    public virtual int V() => 33;
+}
+
+public class D : B
+{
+    public override int V() => 44;
+}
+
+class Runtime_70802
+{
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static void G() {}
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static int F(B b, int n = 10, int m = 10)
+    {
+        int i = 0;
+        int j = 0;
+        int r = 0;
+        goto mid;
+        top:
+        G();
+        mid:
+        r += b.V();
+        i++;
+        if (i < n) goto top;
+        j++;
+        if (i < m) goto mid;
+        return r;     
+    }
+
+    public static int Main()
+    {
+        D d = new D();
+        
+        for (int i = 0; i < 100; i++)
+        {
+            _ = F(d);
+            Thread.Sleep(15);
+        }
+
+        Thread.Sleep(50);
+
+        int r = 0;
+
+        for (int i = 0; i < 100; i++)
+        {
+            r += F(d);
+        }
+
+        Console.WriteLine($"result is {r} (expected 44000)");
+
+        return r / 440;
+    }
+}
diff --git a/src/tests/JIT/opt/Cloning/Runtime_70802.csproj b/src/tests/JIT/opt/Cloning/Runtime_70802.csproj
new file mode 100644
index 00000000000..6bcd0b60a29
--- /dev/null
+++ b/src/tests/JIT/opt/Cloning/Runtime_70802.csproj
@@ -0,0 +1,19 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+  <PropertyGroup>
+    <CLRTestBatchPreCommands><![CDATA[
+$(CLRTestBatchPreCommands)
+set COMPlus_TieredPGO=1
+]]></CLRTestBatchPreCommands>
+    <BashCLRTestPreCommands><![CDATA[
+$(BashCLRTestPreCommands)
+export COMPlus_TieredPGO=1
+]]></BashCLRTestPreCommands>
+  </PropertyGroup>
+</Project>
-- 
GitLab