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 blocks until simple lower #93371

Merged
merged 6 commits into from
Oct 18, 2023

Conversation

AndyAyersMS
Copy link
Member

@AndyAyersMS AndyAyersMS commented Oct 11, 2023

Defer creating throw helper blocks until just before the late flow graph optimizations. This removes some of the flow graph updates from morph, with the aim of making it easier to show that an RPO traversal in morph is safe.

Unfortunately, it's hard to get these blocks in the exact same places they were before, so there are quite a few diffs. They are generally improvements. I also saw some cases where the early creation of throw helpers in try regions inhibited loop recognition; this no longer happens, and may enable loop cloning (and hence size increses).

Diffs

This also lays the groundwork for removing simple lowering.

Don't create any throw helper blocks until simple lowering. This removes
some of the flow graph updates from morph, with the am of making it easier
to show that an RPO traversal in morph is safe.

Unfortunately it's hard to get these blocks in the exact same places
they were before, so there are some diffs.
@ghost ghost assigned AndyAyersMS Oct 11, 2023
@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 Oct 11, 2023
@ghost
Copy link

ghost commented Oct 11, 2023

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

Issue Details

Don't create any throw helper blocks until simple lowering. This removes some of the flow graph updates from morph, with the aim of making it easier to show that an RPO traversal in morph is safe.

Unfortunately it's hard to get these blocks in the exact same places they were before, so there are some diffs.

Author: AndyAyersMS
Assignees: AndyAyersMS
Labels:

area-CodeGen-coreclr

Milestone: -

@AndyAyersMS AndyAyersMS marked this pull request as ready for review October 17, 2023 18:18
@AndyAyersMS
Copy link
Member Author

cc @dotnet/jit-contrib

Code size improvements here are somewhat surprising.... highlighting the need for a global layout algorithm, as there are multiple considerations to determine optimal placements of blocks.

@AndyAyersMS
Copy link
Member Author

SPMI replay errors look like #93527

@EgorBo
Copy link
Member

EgorBo commented Oct 18, 2023

Nice wins! I presume it should fix various cases where we have unreachable throw helper blocks, e,g. I've just came up with:

void Test(bool cond)
{
    int b = 10;
    if (cond)
    {
        _ = checked((byte)b);
    }
}

Emits:

G_M12030_IG01:  ;; offset=0x0000
       sub      rsp, 40
						;; size=4 bbWeight=1 PerfScore 0.25
G_M12030_IG02:  ;; offset=0x0004
       add      rsp, 40
       ret      
						;; size=5 bbWeight=1 PerfScore 1.25
G_M12030_IG03:  ;; offset=0x0009
       call     CORINFO_HELP_OVERFLOW
       int3     
						;; size=6 bbWeight=0 PerfScore 0.00

; Total bytes of code 15

@AndyAyersMS
Copy link
Member Author

presume it should fix various cases where we have unreachable throw helper blocks

Actually, no... let me look into this as a follow-up.

@AndyAyersMS
Copy link
Member Author

presume it should fix various cases where we have unreachable throw helper blocks

Actually, no... let me look into this as a follow-up.

Unfortunately, this seems tricky. We don't know if we really need a throw helper until after codegen, and by then it's not so easy to get rid of things. I suppose I can at least see how often a helper ends up being unused, if it turns out this is a common thing, we can find a way to fix it.

@jakobbotsch
Copy link
Member

It looks like this PR had a ton of misses, so the diffs may be even greater (depending on the reason for the misses I suppose).

@AndyAyersMS
Copy link
Member Author

It looks like this PR had a ton of misses, so the diffs may be even greater (depending on the reason for the misses I suppose).

It could be a bad sign, like the JIT is asking for throw helpers now that it didn't ask for before. Maybe once we have new collections we can do a trial revert and see how that looks.

@AndyAyersMS
Copy link
Member Author

PMI diffs show some new unnecessary throw helper blocks, eg

image

So let me dig into this and see how we can avoid adding such things.

AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this pull request Oct 23, 2023
`compUsesThrowHelper` indicates codegen will likely need to create a call to a helper,
whether or not such calls are shared.

In dotnet#93371 I didn't appreciate this, and so the JIT was failing to set `compUsesThrowHelper`
in cases where a throw helper was going to be needed but throw helper calls were not going to
be shared.

Fixes dotnet#93710.
AndyAyersMS added a commit that referenced this pull request Oct 24, 2023
`compUsesThrowHelper` indicates codegen will likely need to create a call to a helper,
whether or not such calls are shared.

In #93371 I didn't appreciate this, and so the JIT was failing to set `compUsesThrowHelper`
in cases where a throw helper was going to be needed but throw helper calls were not going to
be shared.

Fixes #93710.
liveans pushed a commit to liveans/dotnet-runtime that referenced this pull request Nov 9, 2023
…93897)

`compUsesThrowHelper` indicates codegen will likely need to create a call to a helper,
whether or not such calls are shared.

In dotnet#93371 I didn't appreciate this, and so the JIT was failing to set `compUsesThrowHelper`
in cases where a throw helper was going to be needed but throw helper calls were not going to
be shared.

Fixes dotnet#93710.
@ghost ghost locked as resolved and limited conversation to collaborators Nov 18, 2023
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.

3 participants