Skip to content

Commit

Permalink
Fix issues with EH in unexecuted loops (#82329)
Browse files Browse the repository at this point in the history
Fix two problems with code in loops that are determined to not execute
that have exception handling clauses:
1. Be more specific in loop unrolling to avoid unrolling even zero-count
loops (which causes the loop to be removed) if there is a block in the
loop with a different handler region than the handler region of the "top"
block. This should only kick in for x86, where handlers are not extracted
as funclets.
2. In Value Numbering, before actually value numbering nodes,
`optComputeLoopSideEffectsOfBlock()` walks all the blocks of all top-level
loops collecting data. Unfortunately, it sets the Value Number of certain
nodes as a way to pass information "up the tree" during processing. Presumably
these "fake" value numbers actually get replaced during actual value numbering
for the tree. However, when we go to actually value number the IR, we only
walk reachable blocks. Thus, in some cases, we would end up with a top-level
"fake" value number on an unreachable statement root, thus leading
`fgDebugCheckExceptionSets()` to check the tree and assert. To fix this,
at the end of the `optComputeLoopSideEffectsOfBlock()`, clear the VN in case
it was previously set.

Fixes #81675
  • Loading branch information
BruceForstall authored Feb 22, 2023
1 parent 65ad2ae commit 85ed809
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 3 deletions.
14 changes: 11 additions & 3 deletions src/coreclr/jit/optimizer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4304,17 +4304,19 @@ PhaseStatus Compiler::optUnrollLoops()
// Heuristic: Estimated cost in code size of the unrolled loop.

{
ClrSafeInt<unsigned> loopCostSz; // Cost is size of one iteration
auto tryIndex = loop.lpTop->bbTryIndex;
ClrSafeInt<unsigned> loopCostSz; // Cost is size of one iteration
const BasicBlock* const top = loop.lpTop;

// Besides calculating the loop cost, also ensure that all loop blocks are within the same EH
// region, and count the number of BBJ_RETURN blocks in the loop.
loopRetCount = 0;
for (BasicBlock* const block : loop.LoopBlocks())
{
if (block->bbTryIndex != tryIndex)
if (!BasicBlock::sameEHRegion(block, top))
{
// Unrolling would require cloning EH regions
// Note that only non-funclet model (x86) could actually have a loop including a handler
// but not it's corresponding `try`, if its `try` was moved due to being marked "rare".
JITDUMP("Failed to unroll loop " FMT_LP ": EH constraint\n", lnum);
goto DONE_LOOP;
}
Expand Down Expand Up @@ -8806,6 +8808,12 @@ bool Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk)
}
}
}

// Clear the Value Number from the statement root node, if it was set above. This is to
// ensure that for those blocks that are unreachable, which we still handle in this loop
// but not during Value Numbering, fgDebugCheckExceptionSets() will skip the trees. Ideally,
// we wouldn't be touching the gtVNPair at all (here) before actual Value Numbering.
stmt->GetRootNode()->gtVNPair.SetBoth(ValueNumStore::NoVN);
}

if (memoryHavoc != emptyMemoryKindSet)
Expand Down
70 changes: 70 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_81675/Runtime_81675.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// 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.5 on 2023-02-06 00:41:34
// Run on X86 Windows
// Seed: 12611629827253727687
// Reduced from 444.9 KiB to 1.1 KiB in 00:13:23
// Hits JIT assert in Release:
// Assertion failed '!"Jump into the middle of handler region"' in 'Program:Main(Fuzzlyn.ExecutionServer.IRuntime)' during 'Unroll loops' (IL size 131; hash 0xade6b36b; FullOpts)
//
// File: D:\a\_work\1\s\src\coreclr\jit\fgdiagnostic.cpp Line: 2609
//
public interface I0
{
}

public class C0 : I0
{
public sbyte F3;
}

public class Runtime_81675
{
public static ushort s_11;
public static bool[][] s_18;
public static C0 s_26;
public static short[] s_42;
public static int Main()
{
short vr7 = default(short);
for (int vr8 = 0; vr8 < 0; vr8++)
{
try
{
System.Console.WriteLine(32767);
}
finally
{
I0 vr9 = new C0();
}
}

for (int vr10 = 0; vr10 < 0; vr10++)
{
if (s_18[0][0])
{
var vr11 = s_26.F3;
}
else
{
System.Console.WriteLine(0);
}

if (!((ushort)(-s_11++) >= 0))
{
vr7 = vr7;
try
{
vr7 = 0;
}
finally
{
vr7 = s_42[0];
}
}
}

return 100;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>

0 comments on commit 85ed809

Please sign in to comment.