Skip to content
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.

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

Add EH filters for generic catch(T) blocks #72721

Merged
merged 30 commits into from
Jul 29, 2022

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jul 23, 2022

Closes #67806
Closes #70400

This PR emits single-block EH filters in front of catch clauses which require runtime lookup for Exception objects

Adds generic exceptions support for NativeAOT

src/coreclr/jit/jiteh.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/jiteh.cpp Outdated Show resolved Hide resolved
@EgorBo EgorBo marked this pull request as ready for review July 24, 2022 12:20
@EgorBo
Copy link
Member Author

EgorBo commented Jul 24, 2022

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr crossgen2

@azure-pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@EgorBo EgorBo changed the title NativeAOT: Generic exceptions Add EH filters for generic catch(T) blocks Jul 24, 2022
@jkotas
Copy link
Member

jkotas commented Jul 24, 2022

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EgorBo EgorBo force-pushed the nativeaot-eh-generic-exception branch from 9d9e2ea to 54763a7 Compare July 24, 2022 16:40
@EgorBo
Copy link
Member Author

EgorBo commented Jul 24, 2022

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr crossgen2, runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

EgorBo and others added 3 commits July 25, 2022 23:39
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

…r.cs

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
}
else
{
runtimeLookup = getTokenHandleTree(&resolvedToken, true);
Copy link
Member

Choose a reason for hiding this comment

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

Just curious if this is measurable for something like Assert.Throws<T> given that it is using the slow runtime lookup here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out Xunit's Assert.Throws doesn't use generic "catch" blocks.

Also, this PR actually improves perf 😮:

static void Main(string[] args) =>
    BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);

[Benchmark]
public void Test()
{
    Throws<DivideByZeroException>(() =>
    {
        int b = 0;
        int x = 100 / b;
    });
}

static void Throws<T>(Action action) where T : Exception
{
    try { action(); }
    catch (T) { return; }
    throw new Exception();
}

by 5%

@EgorBo
Copy link
Member Author

EgorBo commented Jul 26, 2022

@dotnet/jit-contrib @AndyAyersMS PTAL jit side
Last run of outerloop/stress pipelines didn't find any issues (https://github.com/dotnet/runtime/runs/7492786872)

lvaTable[tempNum].lvType = TYP_REF;
GenTree* argAsg = gtNewTempAssign(tempNum, arg);
arg = gtNewLclvNode(tempNum, TYP_REF);
filterBb->bbStkTempsIn = tempNum;
Copy link
Member

Choose a reason for hiding this comment

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

Nit -- you don't need to set this.

Suggested change
filterBb->bbStkTempsIn = tempNum;

Copy link
Member Author

Choose a reason for hiding this comment

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

hm.. I think I had to actually, let me see if CI is happy then

Co-authored-by: Andy Ayers <andya@microsoft.com>
@EgorBo
Copy link
Member Author

EgorBo commented Jul 28, 2022

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr crossgen2

@azure-pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@EgorBo EgorBo merged commit 4cd3e78 into dotnet:main Jul 29, 2022
@EgorBo EgorBo deleted the nativeaot-eh-generic-exception branch July 29, 2022 21:29
@ghost ghost locked as resolved and limited conversation to collaborators Aug 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MongoDB.Driver] some methods throw exception after native compiler Support catch(T) in shared code
6 participants