From 96b57915c86d533eb2600a54cfbf07db54c2bce0 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Thu, 13 Aug 2026 18:02:52 +0200 Subject: [PATCH] JIT: don't use a jump-threaded block's sharpened predicate for dominator-based inference When RBO jump threads through a block, it reroutes some of the block's preds directly to the block's successors. If the block is left with a single (ambiguous) pred, optJumpThreadCore sharpens the block's predicate VN to the value flowing in from that pred. The preds that were rerouted, however, were classified against the *old* VN, so the sharpened predicate does not hold on the paths that now bypass the block. Dominator info is not updated as we thread, so the bypassed block still looks like a dominator of its successors, and optRedundantBranch could use its sharpened predicate to fold a branch in a block that is also reachable via the rerouted edges. In the reported case this removed the null check on an isinst result, so an "is MergeFile" arm was entered with a null value. Flag such blocks with BBF_STALE_PREDICATE and skip them in the two dominator-based inference walks in this phase. Fixes #130700 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 049f4632-06bf-47b3-810f-437d77d5938d --- src/coreclr/jit/block.cpp | 1 + src/coreclr/jit/block.h | 2 + src/coreclr/jit/redundantbranchopts.cpp | 31 ++- .../JitBlue/Runtime_130700/Runtime_130700.cs | 207 ++++++++++++++++++ .../Runtime_130700/Runtime_130700.csproj | 14 ++ 5 files changed, 254 insertions(+), 1 deletion(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_130700/Runtime_130700.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_130700/Runtime_130700.csproj diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index 27711d0dc4e56e..d7bfc21e46d0b3 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -550,6 +550,7 @@ void BasicBlock::dspFlags() const {BBF_ASYNC_RESUMPTION, "a-resume"}, {BBF_CATCH_RESUMPTION, "c-resume"}, {BBF_THROW_HELPER, "throw-hlpr"}, + {BBF_STALE_PREDICATE, "stale-pred"}, }; bool first = true; diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 5d12efdc0888cf..c8fec368a6bfa6 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -447,6 +447,8 @@ enum BasicBlockFlags : uint64_t BBF_ASYNC_RESUMPTION = MAKE_BBFLAG(36), // Block is a resumption block in an async method BBF_CATCH_RESUMPTION = MAKE_BBFLAG(37), // Block is a resumption from a catch BBF_THROW_HELPER = MAKE_BBFLAG(38), // Block is a call to a throw helper + BBF_STALE_PREDICATE = MAKE_BBFLAG(39), // Block's branch condition VN only describes flow that + // actually passes through the block (set/used by RBO) // The following are sets of flags. diff --git a/src/coreclr/jit/redundantbranchopts.cpp b/src/coreclr/jit/redundantbranchopts.cpp index cef06b995293f8..09fa0d7b752bb9 100644 --- a/src/coreclr/jit/redundantbranchopts.cpp +++ b/src/coreclr/jit/redundantbranchopts.cpp @@ -104,6 +104,17 @@ PhaseStatus Compiler::optRedundantBranches() OptRedundantBranchesDomTreeVisitor visitor(this); visitor.WalkTree(m_domTree); + // BBF_STALE_PREDICATE is only meaningful while this phase runs, since it is tied to + // the dominator info we started with. Clear it so a later run sees a clean slate. + // + if (visitor.madeChanges) + { + for (BasicBlock* const block : Blocks()) + { + block->RemoveFlags(BBF_STALE_PREDICATE); + } + } + #if DEBUG if (verbose && visitor.madeChanges) { @@ -896,6 +907,12 @@ bool Compiler::optRedundantDominatingBranch(BasicBlock* const block) break; } + if (domBlockProbe->HasFlag(BBF_STALE_PREDICATE)) + { + JITDUMP("failed -- dominator " FMT_BB " has a stale predicate\n", domBlockProbe->bbNum); + break; + } + currentBlock = skipSideEffectFreeBlocks(currentBlock); // Make sure this conditional dominator branches to the same @@ -1228,7 +1245,10 @@ bool Compiler::optRedundantBranch(BasicBlock* const block) // Check the current dominator // - if (domBlock->KindIs(BBJ_COND)) + // Blocks flagged BBF_STALE_PREDICATE are skipped: flow was rerouted around them, so + // their condition no longer holds on every path reaching the blocks they appear to dominate. + // + if (domBlock->KindIs(BBJ_COND) && !domBlock->HasFlag(BBF_STALE_PREDICATE)) { Statement* const domJumpStmt = domBlock->lastStmt(); GenTree* const domJumpTree = domJumpStmt->GetRootNode(); @@ -2677,6 +2697,15 @@ bool Compiler::optJumpThreadCore(JumpThreadInfo& jti) ValueNum treeNewVN = vnStore->VNWithExc(jti.m_ambiguousVN, treeExcVN); tree->SetVN(VNK_Liberal, treeNewVN); + // The preds we just redirected were classified using the old VN, so each of them + // still reaches the successor that the old predicate implies. The sharpened VN, + // however, only describes flow coming from ambBlock, and that is no longer the only + // flow reaching block's successors. Since dominator info is not updated as we thread, + // block can still look like a dominator of those successors, so flag it to keep the + // rest of this phase from inferring anything from its now path-specific predicate. + // + jti.m_block->SetFlags(BBF_STALE_PREDICATE); + JITDUMP("Updating [%06u] liberal VN from " FMT_VN " to " FMT_VN "\n", dspTreeID(tree), treeOldVN, treeNewVN); } diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_130700/Runtime_130700.cs b/src/tests/JIT/Regression/JitBlue/Runtime_130700/Runtime_130700.cs new file mode 100644 index 00000000000000..696f0cb6a19c51 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_130700/Runtime_130700.cs @@ -0,0 +1,207 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// Redundant branch opts jump-threaded flow around a block and sharpened that block's +// predicate VN, but left dominator info claiming the block still dominated its successors. +// A later dominator-based inference then folded away the null check on an 'isinst' result, +// so the 'is MergeFile' arm below was entered with a null value. + +#nullable enable + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Runtime.CompilerServices; +using System.Threading; +using Xunit; + +namespace Runtime_130700; + +internal static class Contract +{ + public static void Assert(bool condition, string? message = "", params object?[] args) + { + if (condition) + { + return; + } + + message ??= string.Empty; + throw new Exception(string.Format(message, args)); + } + + public static void Fail(string? message = "", params object?[] args) + { + message ??= string.Empty; + throw new Exception(string.Format(message, args)); + } + + public static T AssertNotNull(T? o, string? message = "", params object?[] args) + where T : class + { + if (o != null) + { + return o; + } + + message ??= string.Empty; + throw new Exception(string.Format(message, args)); + } +} + +internal static class EnumerableExtensions +{ + public static void ForEach(this IEnumerable? enumeration, Action? action) + { + if (enumeration == null || action == null) + { + return; + } + + foreach (T item in enumeration) + { + action(item); + } + } +} + +public abstract class MergeHierarchyMember +{ +} + +public class MergeFile : MergeHierarchyMember +{ + public string FileName + { + get => field; + set + { + Contract.Assert(!string.IsNullOrWhiteSpace(value)); + field = value; + } + } + + public MergeFile(string fileName) + { + Contract.Assert(fileName.All(c => c != Path.DirectorySeparatorChar && c != Path.AltDirectorySeparatorChar), + "FileName must not contain any directory separator characters"); + FileName = fileName; + } +} + +public class MergeHierarchy : MergeHierarchyMember +{ + public enum MergeMode + { + Automatic, + Manual, + PostProcessing + } + + public MergeMode Mode { get; set; } + + public List Children { get; set; } = new(); + + public MergeHierarchy(MergeMode mergeMode) + { + Mode = mergeMode; + } + + public MergeHierarchy(MergeMode mergeMode, params (MergeHierarchyMember Member, long Offset)[] membersWithOffsets) + : this(mergeMode) + { + Contract.AssertNotNull(membersWithOffsets); + membersWithOffsets.ForEach(elem => AddMergeHierarchyMember(elem.Member, elem.Offset)); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + internal void AddMergeHierarchyMember(MergeHierarchyMember mergeHierarchyMember, long offset) + { + Contract.AssertNotNull(mergeHierarchyMember); + Contract.Assert(mergeHierarchyMember is not MergeHierarchy { Mode: MergeMode.PostProcessing }, + "Post-processing can only be the root node of a merge hierarchy"); + + // 'mergeHierarchyMember is MergeFile' is only evaluated when Mode is Automatic. + Contract.Assert(Mode is not MergeMode.Automatic || (Mode is MergeMode.Automatic && mergeHierarchyMember is MergeFile), + "An automatic merge hierarchy cannot be nested in another automatic merge hierarchy"); + + Children.Add(new ChildWithOffset(mergeHierarchyMember, offset)); + } +} + +public class ChildWithOffset +{ + public enum MemberKind + { + MergeFile, + MergeHierarchy, + } + + public MemberKind Kind { get; set; } + + public MergeHierarchy? MergeHierarchy { get; set; } + + public MergeFile? MergeFile + { + get => field; + set + { + Contract.Assert(value is null || !string.IsNullOrWhiteSpace(value.FileName)); + field = value; + } + } + + public long Offset { get; set; } + + public ChildWithOffset(MergeHierarchyMember mergeHierarchyMember, long offset) + { + switch (mergeHierarchyMember) + { + case MergeFile mergeFile: + Kind = MemberKind.MergeFile; + MergeFile = mergeFile; + break; + case MergeHierarchy mergeHierarchy: + Kind = MemberKind.MergeHierarchy; + MergeHierarchy = mergeHierarchy; + break; + default: + Contract.Fail("Unknown MergeHierarchyMember"); + break; + } + + Offset = offset; + } +} + +public class Runtime_130700 +{ + [Fact] + public static void TestEntryPoint() + { + // Fixed sub-hierarchy, so nothing is retained across iterations. + MergeHierarchy inner = new MergeHierarchy( + MergeHierarchy.MergeMode.Manual, (new MergeFile("hello"), 0), (new MergeFile("world"), 0)); + + // AddMergeHierarchyMember has to reach tier-1 with profile data, so keep calling it + // (with both a MergeHierarchy and a MergeFile argument) while it tiers up. + for (int round = 0; round < 100; round++) + { + for (int i = 0; i < 1000; i++) + { + MergeHierarchy hierarchy = new MergeHierarchy(MergeHierarchy.MergeMode.Manual, (inner, i)); + hierarchy.AddMergeHierarchyMember(new MergeFile($"file{i}"), offset: i); + + if (hierarchy.Children[0].Kind != ChildWithOffset.MemberKind.MergeHierarchy || + hierarchy.Children[1].Kind != ChildWithOffset.MemberKind.MergeFile) + { + Assert.Fail($"Wrong member kinds at round {round}, iteration {i}: " + + $"{hierarchy.Children[0].Kind}, {hierarchy.Children[1].Kind}"); + } + } + + Thread.Sleep(1); + } + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_130700/Runtime_130700.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_130700/Runtime_130700.csproj new file mode 100644 index 00000000000000..c8b6bc7450ed20 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_130700/Runtime_130700.csproj @@ -0,0 +1,14 @@ + + + True + 1 + + true + + + + + + + +