Skip to content

JIT: fix try region cloning when try is nested in a handler #111975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 29, 2025
Merged

JIT: fix try region cloning when try is nested in a handler #111975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 18 additions & 8 deletions src/coreclr/jit/fgehopt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2761,14 +2761,20 @@ BasicBlock* Compiler::fgCloneTryRegion(BasicBlock* tryEntry, CloneTryInfo& info,
// this is cheaper than any other insertion point, as no existing regions get renumbered.
//
unsigned insertBeforeIndex = enclosingTryIndex;
if (insertBeforeIndex == EHblkDsc::NO_ENCLOSING_INDEX)
if ((enclosingTryIndex == EHblkDsc::NO_ENCLOSING_INDEX) && (enclosingHndIndex == EHblkDsc::NO_ENCLOSING_INDEX))
{
JITDUMP("Cloned EH clauses will go at the end of the EH table\n");
JITDUMP("No enclosing EH region; cloned EH clauses will go at the end of the EH table\n");
insertBeforeIndex = compHndBBtabCount;
}
else if ((enclosingTryIndex == EHblkDsc::NO_ENCLOSING_INDEX) || (enclosingHndIndex < enclosingTryIndex))
{
JITDUMP("Cloned EH clauses will go before enclosing handler region EH#%02u\n", enclosingHndIndex);
insertBeforeIndex = enclosingHndIndex;
}
else
{
JITDUMP("Cloned EH clauses will go before enclosing region EH#%02u\n", enclosingTryIndex);
JITDUMP("Cloned EH clauses will go before enclosing try region EH#%02u\n", enclosingTryIndex);
assert(insertBeforeIndex == enclosingTryIndex);
}

// Once we call fgTryAddEHTableEntries with deferCloning = false,
Expand Down Expand Up @@ -2989,7 +2995,7 @@ BasicBlock* Compiler::fgCloneTryRegion(BasicBlock* tryEntry, CloneTryInfo& info,
const unsigned originalTryIndex = block->getTryIndex();
unsigned cloneTryIndex = originalTryIndex;

if (originalTryIndex <= outermostTryIndex)
if (originalTryIndex < enclosingTryIndex)
{
cloneTryIndex += indexShift;
}
Expand All @@ -3003,11 +3009,15 @@ BasicBlock* Compiler::fgCloneTryRegion(BasicBlock* tryEntry, CloneTryInfo& info,
if (block->hasHndIndex())
{
const unsigned originalHndIndex = block->getHndIndex();
unsigned cloneHndIndex = originalHndIndex;

if (originalHndIndex < enclosingHndIndex)
{
cloneHndIndex += indexShift;
}

// if (originalHndIndex ==
const unsigned cloneHndIndex = originalHndIndex + indexShift;
EHblkDsc* const originalEbd = ehGetDsc(originalHndIndex);
EHblkDsc* const clonedEbd = ehGetDsc(cloneHndIndex);
EHblkDsc* const originalEbd = ehGetDsc(originalHndIndex);
EHblkDsc* const clonedEbd = ehGetDsc(cloneHndIndex);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these ehGetDsc calls dead code? Ditto the ehGetDsc calls above for the try region logic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, looks like they are.

This code has been churned quite a bit... I am still fighting off the temptation to revise the entire EH model since it is currently not update friendly. In retrospect perhaps that would have been an easier path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix that in a subsequent PR.

newBlock->setHndIndex(cloneHndIndex);
updateBlockReferences(cloneHndIndex);

Expand Down
204 changes: 197 additions & 7 deletions src/tests/JIT/opt/Cloning/loops_with_eh.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
using System.Runtime.CompilerServices;
using Xunit;


// Cheat codes
//
// L - loop
Expand All @@ -17,6 +16,7 @@
// m - multiple try exits (TF will remain a try finally)
// g - giant finally (TF will remain try finally)
// p - regions are serial, not nested
// TFi - try finally with what follows in the finally
//
// x: we currently cannot clone loops where the try is the first thing
// as the header and preheader are different regions
Expand All @@ -26,6 +26,9 @@ public class LoopsWithEH
static int[] data;
static int n;

[MethodImpl(MethodImplOptions.NoInlining)]
static void SideEffect() { }

static LoopsWithEH()
{
data = new int[100];
Expand Down Expand Up @@ -178,7 +181,7 @@ public static int Sum_LxTCC(int[] data, int n)
{
return -1;
}
catch(Exception)
catch (Exception)
{
return -2;
}
Expand All @@ -201,7 +204,7 @@ public static int Sum_LxTCcC(int[] data, int n)
}
catch (IndexOutOfRangeException)
{
sum +=1;
sum += 1;
}
catch (Exception)
{
Expand Down Expand Up @@ -536,7 +539,7 @@ public static int Sum_TCLxTfC(int[] data, int n)
}
}
}
catch (Exception)
catch (Exception)
{
return -1;
}
Expand Down Expand Up @@ -642,7 +645,7 @@ public static int Sum_TCLxTF(int[] data, int n)
{
sum += data[i];
}
finally
finally
{
sum += 1;
}
Expand Down Expand Up @@ -807,7 +810,7 @@ public static int Sum_LxTFTF(int[] data, int n)

[Fact]
public static int Test_LxTFxTF() => Sum_LxTFTF(data, n) - 110;

public static int Sum_TFLxTF(int[] data, int n)
{
int sum = 0;
Expand Down Expand Up @@ -896,7 +899,7 @@ public static int Sum_TCTFLxTF(int[] data, int n)
sum += 1;
}
}
catch(Exception)
catch (Exception)
{
return -1;
}
Expand Down Expand Up @@ -937,5 +940,192 @@ public static int Sum_TFTCLxTF(int[] data, int n)
}
return sum;
}

[Fact]
public static int Test_TFiL() => Sum_TFiL(data, n) - 91;

public static int Sum_TFiL(int[] data, int n)
{
int sum = 0;
try
{
SideEffect();
}
finally
{
sum += 1;
for (int i = 0; i < n; i++)
{
sum += data[i];
}
}

return sum;
}

[Fact]
public static int Test_TFiLxTF() => Sum_TFiLxTF(data, n) - 131;

public static int Sum_TFiLxTF(int[] data, int n)
{
int sum = 0;
try
{
SideEffect();
}
finally
{
sum += 1;
for (int i = 0; i < n; i++)
{
sum += 1;
try
{
sum += data[i];
}
finally
{
sum += 1;
}
}
}

return sum;
}

[Fact]
public static int Test_TFiLxTCc() => Sum_TFiLxTCc(data, n) - 111;

public static int Sum_TFiLxTCc(int[] data, int n)
{
int sum = 0;
try
{
SideEffect();
}
finally
{
sum += 1;
for (int i = 0; i < n; i++)
{
sum += 1;
try
{
sum += data[i];
}
catch (Exception)
{
sum += 1;
}
}
}

return sum;
}

[Fact]
public static int Test_TFiLxTC() => Sum_TFiLxTC(data, n) - 112;

public static int Sum_TFiLxTC(int[] data, int n)
{
int sum = 0;
try
{
SideEffect();
}
finally
{
sum += 1;
for (int i = 0; i < n; i++)
{
sum += 1;
try
{
sum += data[i];
}
catch (Exception)
{
goto after_loop;
}
}

after_loop:
sum += 1;

}

return sum;
}

[Fact]
public static int Test_TFTFiLxTC() => Sum_TFTFiLxTC(data, n) - 113;

public static int Sum_TFTFiLxTC(int[] data, int n)
{
int sum = 0;
try
{
try
{
SideEffect();
}
finally
{
sum += 1;
for (int i = 0; i < n; i++)
{
sum += 1;
try
{
sum += data[i];
}
catch (Exception)
{
goto after_loop;
}
}

after_loop:
sum += 1;
}
}
finally
{
sum += 1;
}

return sum;
}


[Fact]
public static int Test_TFiTFxL() => Sum_TFiTFxL(data, n) - 92;

public static int Sum_TFiTFxL(int[] data, int n)
{
int sum = 0;

try
{
SideEffect();
}
finally
{
try
{
sum += 1;
for (int i = 0; i < n; i++)
{
sum += data[i];
}
}
finally
{
sum += 1;
}
}

return sum;
}
}

Loading