Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/coreclr/jit/block.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/jit/block.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
31 changes: 30 additions & 1 deletion src/coreclr/jit/redundantbranchopts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
}

Expand Down
207 changes: 207 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_130700/Runtime_130700.cs
Original file line number Diff line number Diff line change
@@ -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>(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<T>(this IEnumerable<T>? enumeration, Action<T>? 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<ChildWithOffset> 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);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<Optimize>True</Optimize>
<CLRTestPriority>1</CLRTestPriority>
<!-- Needed for CLRTestEnvironmentVariable -->
<RequiresProcessIsolation>true</RequiresProcessIsolation>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
<!-- The issue reproduces only with tiered compilation and PGO enabled -->
<CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="1" />
<CLRTestEnvironmentVariable Include="DOTNET_TieredPGO" Value="1" />
</ItemGroup>
</Project>
Loading