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

JIT: defer creating throw helper code until we know it's needed #104819

Merged
merged 2 commits into from
Jul 13, 2024

Conversation

AndyAyersMS
Copy link
Member

@AndyAyersMS AndyAyersMS commented Jul 12, 2024

Defer creation of throw helper code until the stack level setter analysis has determined the throw helper is needed. Defer finalizing the outgoing arg size as well.

The throw helper support now goes through the steps:

  • early on phases can request that a throw helper block get created
  • we create the blocks (but not the code) before final block layout so they get proper placement
  • lowering adds references to throw helpers that are actually needed
  • stack level setter then materializes code for the needed helpers, and removes the blocks for the unneeded ones
  • we can then finalize the outgoing arg space size

Closes #104658.

Diffs

Defer creation of throw helper code until the stack level setter analysis has\
determined the throw helper is needed. Defer finalizing the outgoing arg size
as well.

The throw helper support now goes through the steps:
* early on phases can request that a throw helper block get created
* we create the blocks (but not the code) before final block layout so
they get proper placement
* lowering adds references to throw helpers that are actually needed
* stack level setter then materializes code for the needed helpers, and
removes the blocks for the unneeded ones
* we can then finalize the outgoing arg space size

Closes dotnet#104658.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 12, 2024
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@AndyAyersMS
Copy link
Member Author

@jakobbotsch PTAL
cc @dotnet/jit-contrib

Some size improvements, no regressions. Oddly, some perf score regressions.

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

LGTM.

Any idea how hard it would be to delay creating the BasicBlock itself as well? We would need to fill in some liveness, but I wonder if there are any other problems with it.

@AndyAyersMS
Copy link
Member Author

LGTM.

Any idea how hard it would be to delay creating the BasicBlock itself as well? We would need to fill in some liveness, but I wonder if there are any other problems with it.

We'd have to do some custom placement instead of relying on the main block layout algorithm. Or defer layout until later.

@jakobbotsch
Copy link
Member

We'd have to do some custom placement instead of relying on the main block layout algorithm. Or defer layout until later.

I see, I didn't know we run the layout algorithm before lowering.

I suppose the important part there is fgMoveColdBlocks, since the RPO part of the layout algorithm is not going to do much for the throw helpers considering that they aren't modelled in the flow graph.

@amanasifkhalid
Copy link
Member

amanasifkhalid commented Jul 12, 2024

If the throw helper's block is assumed cold, then it should be pretty trivial to emulate fgMoveColdBlocks's behavior by just inserting the new throw block at the end of whatever region you're in, right? Moving cold blocks to the end of each region already prevents the hot path from falling out of the region, so I don't see any additional penalty from just appending the throw helper block to the end of the region.

@AndyAyersMS
Copy link
Member Author

it should be pretty trivial to emulate fgMoveColdBlocks's behavior

My preference would be to have just one place that does all the layout logic. In particular once we start splitting, and if we're ever able to split EH regions, this stuff will get more involved.

@amanasifkhalid
Copy link
Member

My preference would be to have just one place that does all the layout logic.

Got it, I still plan on exploring moving layout (and fgDetermineFirstColdBlock) further back, so it's probably best to not introduce more localized layout changes.

In particular once we start splitting

I appreciate the commitment your usage of "once" implies

@LoopedBard3
Copy link
Member

Potential regression(s): @AndyAyersMS
Windows x64: dotnet/perf-autofiling-issues#38298

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JIT: Unnecessary prolog frame setup in .NET 9
4 participants