Skip to content

Commit

Permalink
Fix setting EH region on new loop header blocks (dotnet#91711)
Browse files Browse the repository at this point in the history 
One loop canonicalization is disallow a loop `bottom` block from being
the `head` block of another loop. When this canonicalization is done,
a new `head` block is inserted above the loop `top` of the `head` block
loop, and it becomes the new `head` block. When doing so, the EH region
of `top` was being extended to include the new `head` block. This is
illegal if the `top` block is the beginning of a `try`, and there are
branches from outside the `try` to the block, as it will create an illegal
branch into the middle of a `try` region.

The solution is to re-use existing code in the creation of loop pre-headers
to set the proper EH region on the new `head` block.

So, the canonicalization does this (example):

```
BB10 // top of loop L00
BB11 -> BB10 // bottom of loop L00; head of loop L01
BB12 // top of loop L01
BB13 -> BB12 // bottom of loop L01
```

Here, we insert a new BB20 to split the dual purpose of BB11, as such:
```
BB10 // top of loop L00
BB11 -> BB10 // bottom of loop L00
BB20 // new head of loop L01
BB12 // top of loop L01
BB13 -> BB12 // bottom of loop L01
```

Note that if BB12 is a single-block `try` region, the new BB20 cannot have
the same `try` region index as BB12, since BB13 branches to BB12. So in this
case, BB20 needs to get the `try` index of the parent EH region of BB12.
  • Loading branch information
@BruceForstall
BruceForstall authored Sep 7, 2023
1 parent 4a18e6f commit 1a0189c
Show file tree
Hide file tree
Showing 4 changed files with 173 additions and 57 deletions.
2 changes: 2 additions & 0 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5488,6 +5488,8 @@ class Compiler

bool fgCreateLoopPreHeader(unsigned lnum);

void fgSetEHRegionForNewLoopHead(BasicBlock* newHead, BasicBlock* top);

void fgUnreachableBlock(BasicBlock* block);

void fgRemoveConditionalJump(BasicBlock* block);
Expand Down
129 changes: 72 additions & 57 deletions src/coreclr/jit/optimizer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3035,7 +3035,9 @@ bool Compiler::optCanonicalizeLoop(unsigned char loopInd)

JITDUMP(FMT_LP " head " FMT_BB " is also " FMT_LP " bottom\n", loopInd, h->bbNum, sibling);

BasicBlock* const newH = fgNewBBbefore(BBJ_NONE, t, /*extendRegion*/ true);
BasicBlock* const newH = fgNewBBbefore(BBJ_NONE, t, /*extendRegion*/ false);

fgSetEHRegionForNewLoopHead(newH, t);

fgRemoveRefPred(t, h);
fgAddRefPred(t, newH);
Expand Down Expand Up @@ -7958,6 +7960,74 @@ bool Compiler::optVNIsLoopInvariant(ValueNum vn, unsigned lnum, VNSet* loopVnInv
return res;
}

//------------------------------------------------------------------------------
// fgSetEHRegionForNewLoopHead: When a new loop HEAD block is created, this sets the EH region properly for
// the new block.
//
// In which EH region should the header live?
//
// The header block is added immediately before `top`.
//
// The `top` block cannot be the first block of a filter or handler: `top` must have a back-edge from a
// BBJ_COND or BBJ_ALWAYS within the loop, and a filter or handler cannot be branched to like that.
//
// The `top` block can be the first block of a `try` region, and you can fall into or branch to the
// first block of a `try` region. (For top-entry loops, `top` will both be the target of a back-edge
// and a fall-through from the previous block.)
//
// If the `top` block is NOT the first block of a `try` region, the header can simply extend the
// `top` block region.
//
// If the `top` block IS the first block of a `try`, we find its parent region and use that. For mutual-protect
// regions, we need to find the actual parent, as the block stores the most "nested" mutual region. For
// non-mutual-protect regions, due to EH canonicalization, we are guaranteed that no other EH regions begin
// on the same block, so looking to just the parent is sufficient. Note that we can't just extend the EH
// region of `top` to the header, because `top` will still be the target of backward branches from
// within the loop. If those backward branches come from outside the `try` (say, only the top half of the loop
// is a `try` region), then we can't branch to a non-first `try` region block (you always must entry the `try`
// in the first block).
//
// Note that hoisting any code out of a try region, for example, to a pre-header block in a different
// EH region, needs to ensure that no exceptions will be thrown.
//
// Arguments:
// newHead - the new `head` block, which has already been added to the block list ahead of the loop `top`
// top - the loop `top` block
//
void Compiler::fgSetEHRegionForNewLoopHead(BasicBlock* newHead, BasicBlock* top)
{
assert(newHead->bbNext == top);
assert(!fgIsFirstBlockOfFilterOrHandler(top));

if ((top->bbFlags & BBF_TRY_BEG) != 0)
{
// `top` is the beginning of a try block. Figure out the EH region to use.
assert(top->hasTryIndex());
unsigned short newTryIndex = (unsigned short)ehTrueEnclosingTryIndexIL(top->getTryIndex());
if (newTryIndex == EHblkDsc::NO_ENCLOSING_INDEX)
{
// No EH try index.
newHead->clearTryIndex();
}
else
{
newHead->setTryIndex(newTryIndex);
}

// What handler region to use? Use the same handler region as `top`.
newHead->copyHndIndex(top);
}
else
{
// `top` is not the beginning of a try block. Just extend the EH region to the pre-header.
// We don't need to call `fgExtendEHRegionBefore()` because all the special handling that function
// does it to account for `top` being the first block of a `try` or handler region, which we know
// is not true.

newHead->copyEHRegion(top);
}
}

//------------------------------------------------------------------------------
// fgCreateLoopPreHeader: Creates a pre-header block for the given loop.
// A pre-header is a block outside the loop that falls through or branches to the loop
Expand Down Expand Up @@ -8196,62 +8266,7 @@ bool Compiler::fgCreateLoopPreHeader(unsigned lnum)

// Link in the preHead block
fgInsertBBbefore(top, preHead);

// In which EH region should the pre-header live?
//
// The pre-header block is added immediately before `top`.
//
// The `top` block cannot be the first block of a filter or handler: `top` must have a back-edge from a
// BBJ_COND or BBJ_ALWAYS within the loop, and a filter or handler cannot be branched to like that.
//
// The `top` block can be the first block of a `try` region, and you can fall into or branch to the
// first block of a `try` region. (For top-entry loops, `top` will both be the target of a back-edge
// and a fall-through from the previous block.)
//
// If the `top` block is NOT the first block of a `try` region, the pre-header can simply extend the
// `top` block region.
//
// If the `top` block IS the first block of a `try`, we find its parent region and use that. For mutual-protect
// regions, we need to find the actual parent, as the block stores the most "nested" mutual region. For
// non-mutual-protect regions, due to EH canonicalization, we are guaranteed that no other EH regions begin
// on the same block, so looking to just the parent is sufficient. Note that we can't just extend the EH
// region of `top` to the pre-header, because `top` will still be the target of backward branches from
// within the loop. If those backward branches come from outside the `try` (say, only the top half of the loop
// is a `try` region), then we can't branch to a non-first `try` region block (you always must entry the `try`
// in the first block).
//
// Note that hoisting any code out of a try region, for example, to a pre-header block in a different
// EH region, needs to ensure that no exceptions will be thrown.

assert(!fgIsFirstBlockOfFilterOrHandler(top));

if ((top->bbFlags & BBF_TRY_BEG) != 0)
{
// `top` is the beginning of a try block. Figure out the EH region to use.
assert(top->hasTryIndex());
unsigned short newTryIndex = (unsigned short)ehTrueEnclosingTryIndexIL(top->getTryIndex());
if (newTryIndex == EHblkDsc::NO_ENCLOSING_INDEX)
{
// No EH try index.
preHead->clearTryIndex();
}
else
{
preHead->setTryIndex(newTryIndex);
}

// What handler region to use? Use the same handler region as `top`.
preHead->copyHndIndex(top);
}
else
{
// `top` is not the beginning of a try block. Just extend the EH region to the pre-header.
// We don't need to call `fgExtendEHRegionBefore()` because all the special handling that function
// does it to account for `top` being the first block of a `try` or handler region, which we know
// is not true.

preHead->copyEHRegion(top);
}
fgSetEHRegionForNewLoopHead(preHead, top);

// TODO-CQ: set dominators for this block, to allow loop optimizations requiring them
// (e.g: hoisting expression in a loop with the same 'head' as this one)
Expand Down
91 changes: 91 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_91108/Runtime_91108.cs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

// Found by Antigen

using System;
using System.Runtime.CompilerServices;
using Xunit;

public class TestClass
{
public struct S1
{
public decimal decimal_0;
}
public struct S2
{
public long long_1;
}
static byte s_byte_3 = 4;
static int s_int_8 = 50;
static long s_long_9 = -2;
static float s_float_11 = 0.4f;
static S2 s_s2_17 = new S2();
int int_24 = 0;
long long_25 = 5;
sbyte sbyte_26 = 1;
S2 s2_33 = new S2();
static int s_loopInvariant = 4;
private int LeafMethod6()
{
unchecked
{
return ((int)(((int)(s_int_8 = ((int)(s_int_8 &= ((int)(s_int_8 % ((int)((s_int_8) | 49)))))))) ^ ((int)(s_int_8 & ((int)(s_int_8 += ((int)(int_24 &= s_int_8))))))));
}
}
private S2 Method22(ref S2 p_s2_543, S2 p_s2_544, ref float p_float_545, out int p_int_546, ref S2 p_s2_547)
{
unchecked
{
int int_554 = -1;
p_int_546 = ((int)(((int)(int_554 |= ((int)(((int)(int_554 |= -1)) + ((int)(int_24 >>= s_int_8)))))) / ((int)((((int)(((int)(((int)(s_int_8 / ((int)((int_24) | 46)))) & int_24)) | ((int)(((int)(int_554 % ((int)((int_554) | 16)))) << ((int)(int_554 % ((int)((int_24) | 52))))))))) | 26))));
int __loopvar0 = s_loopInvariant - 7;
int __loopvar3 = s_loopInvariant - 15;
for (;;)
{
if (__loopvar3 > s_loopInvariant + 4)
break;
int __loopvar2 = s_loopInvariant;
for (; (__loopvar2 > s_loopInvariant - 4); __loopvar2--)
{
try
{
s_byte_3 -= s_byte_3;
}
finally
{
sbyte_26 <<= ((int)(LeafMethod6() << ((int)(s_int_8 + ((int)(s_int_8 = ((int)(p_int_546 -= p_int_546))))))));
++__loopvar3;
}
long long_565 = ((long)(((long)(((long)(((long)(s_long_9 + long_25)) << ((int)(s_int_8 % ((int)((int_24) | 22)))))) >> ((int)(((int)(int_24 + LeafMethod6())) + ((int)(int_24 % ((int)((s_int_8) | 81)))))))) << int_554));
}
}
return s2_33;
}
}
private void Method0()
{
unchecked
{
int int_2518 = 50;
s_s2_17 = Method22(ref s2_33, s_s2_17, ref s_float_11, out int_2518, ref s_s2_17);
}
}
[Fact]
public static void TestEntryPoint()
{
new TestClass().Method0();
}
}
/*
Environment:
set COMPlus_TieredCompilation=0
Jump into the middle of try region: BB07 branches to BB06
Assert failure(PID 38788 [0x00009784], Thread: 44976 [0xafb0]): Assertion failed '!"Jump into middle of try region"' in 'TestClass:Method22(byref,TestClass+S2,byref,byref,byref):TestClass+S2:this' during 'Find loops' (IL size 287; hash 0xf1c30f9f; FullOpts)
File: C:\gh\runtime\src\coreclr\jit\fgdiagnostic.cpp Line: 2624
*/
8 changes: 8 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_91108/Runtime_91108.csproj
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>

0 comments on commit 1a0189c

Please sign in to comment.