From bc0706341d38ab54df0d45a932f105877ab9e5b1 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Wed, 8 Jul 2026 00:02:31 +0200 Subject: [PATCH] JIT: fix widened IV init placed in unreachable block (#130190) Fixes #130189. `optWidenPrimaryIV` initializes the widened IV in the block that holds the narrow IV's reaching def when that block is not the preheader. Redundant branch opts' dominator-based jump threading can bypass that def block on the path that reaches the loop while its SSA def still reaches the loop (stale SSA). The widened IV init was then emitted into that (now dead) block, which a later flow-graph pass removes, so the widened IV was never initialized and the loop produced a wrong result. Fix: only use the reaching-def block for the init when the reaching def is **not** a PHI. RBO's jump threading only leaves stale SSA at join points (PHIs), so a PHI reaching def is exactly the case that can no longer reach the loop; a non-PHI def always dominates its uses and therefore reaches the loop. When the reaching def is a PHI we fall back to the preheader, which always reaches the loop. A DFS-reachability check (`m_dfsTree->Contains`) is not sufficient here: a PHI's block can remain reachable via another path while the path that reaches the loop was threaded away, so it would still be selected even though it no longer reaches the loop. The PHI-def condition does not have that hole. Notes: - The root cause is stale SSA left by RBO dominator-based jump threading. Making RBO bail in that case is correct but pessimizes many cases where the stale SSA is harmless, so this instead hardens the only consumer that trips on it (IV widening). - SPMI (x64) shows this only moves a handful of widened-IV inits into the preheader: +85 bytes over ~676K contexts (libraries.pmi/crossgen2/aspnet2/realworld), mostly neutral placement churn, some improvements. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jakob Botsch Nielsen (cherry picked from commit 5f446517e1d234ed201a7311fd74ef6a46098c74) --- src/coreclr/jit/inductionvariableopts.cpp | 6 ++- .../JitBlue/Runtime_130189/Runtime_130189.cs | 52 +++++++++++++++++++ .../Runtime_130189/Runtime_130189.csproj | 9 ++++ 3 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_130189/Runtime_130189.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_130189/Runtime_130189.csproj diff --git a/src/coreclr/jit/inductionvariableopts.cpp b/src/coreclr/jit/inductionvariableopts.cpp index e107701219edc6..464e533adc8997 100644 --- a/src/coreclr/jit/inductionvariableopts.cpp +++ b/src/coreclr/jit/inductionvariableopts.cpp @@ -923,7 +923,11 @@ bool Compiler::optWidenPrimaryIV(FlowGraphNaturalLoop* loop, unsigned lclNum, Sc BasicBlock* preheader = loop->EntryEdge(0)->getSourceBlock(); BasicBlock* initBlock = preheader; - if ((startSsaDsc->GetBlock() != nullptr) && (startSsaDsc->GetDefNode() != nullptr)) + // Prefer to initialize the widened IV in the same block as the reaching def + // of the narrow IV, but only if the reaching def is not a phi. RBO's jump threading + // can leave stale SSA with the once-containing block being unreachable. + if ((startSsaDsc->GetBlock() != nullptr) && (startSsaDsc->GetDefNode() != nullptr) && + !startSsaDsc->GetDefNode()->IsPhiDefn()) { initBlock = startSsaDsc->GetBlock(); } diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_130189/Runtime_130189.cs b/src/tests/JIT/Regression/JitBlue/Runtime_130189/Runtime_130189.cs new file mode 100644 index 00000000000000..f5b79aa5ea4b4f --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_130189/Runtime_130189.cs @@ -0,0 +1,52 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.CompilerServices; +using Xunit; + +// Regression test for https://github.com/dotnet/runtime/issues/130189 +// +// Redundant branch opts' dominator-based jump threading used to bypass a block +// that contained a globally-used PHI def without updating SSA. That left the +// loop reading a stale (soon to be dead) PHI value, which subsequent induction +// variable widening then miscompiled, producing a wrong result. + +public sealed class Runtime_130189 +{ + ushort[] d = [0, 0, 1, 65535, 1, 0, 1, 65535, 65535]; + int[] a = [0, 4]; + int i; + + bool Next(out int p) + { + bool r; + if (r = i < a.Length) + p = a[i++]; + else + p = -1; + return r; + } + + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)] + internal long Sum() + { + i = 0; + long s = 0; + int x, p; + while (Next(out p)) + { + while ((x = d[p]) != 65535) + { + s += x; + p++; + } + } + return s; + } + + [Fact] + public static void TestEntryPoint() + { + Assert.Equal(3, new Runtime_130189().Sum()); + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_130189/Runtime_130189.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_130189/Runtime_130189.csproj new file mode 100644 index 00000000000000..501217e4d86892 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_130189/Runtime_130189.csproj @@ -0,0 +1,9 @@ + + + None + True + + + + +