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: Run optRedundantBranches twice and try to compute more accurate doms #70907

Merged
merged 18 commits into from
Jul 2, 2022

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jun 17, 2022

Closes #70480

void Test(object o)
{
    if (o is int n)
        Console.WriteLine(n);
}

Asm diff for ^: https://www.diffchecker.com/8j7VGRuz

image

BB04 and BB07 are unreachable, because of "dead" BB04, BB06 reports BB01 as a dominator instead of BB02

Some nice spmi diffs

@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 Jun 17, 2022
@ghost ghost assigned EgorBo Jun 17, 2022
@ghost
Copy link

ghost commented Jun 17, 2022

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

Issue Details

Closes #70480

void Test(object o)
{
    if (o is int n)
        Console.WriteLine(n);
}

Asm diff for ^: https://www.diffchecker.com/8j7VGRuz

image

Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo EgorBo marked this pull request as ready for review June 17, 2022 23:35
@EgorBo
Copy link
Member Author

EgorBo commented Jun 18, 2022

PTAL @AndyAyersMS @dotnet/jit-contrib
Diffs up to

Total bytes of delta: -54496 (-0.08 % of base)

for libraries.pmi.windows.arm64.checked.mch

@EgorBo
Copy link
Member Author

EgorBo commented Jun 18, 2022

/azp run runtime-coreclr outerloop, fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

I like the idea, but it seems kind of costly.

src/coreclr/jit/flowgraph.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/redundantbranchopts.cpp Outdated Show resolved Hide resolved
@jakobbotsch
Copy link
Member

Antigen has better coverage for loops than Fuzzlyn, so you may also want to consider running that (although I do see Fuzzlyn found something).

@EgorBo
Copy link
Member Author

EgorBo commented Jun 20, 2022

/azp run fuzzlyn, antigen

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@AndyAyersMS
Copy link
Member

Are the fuzzlyn/antigen failures real?

@EgorBo
Copy link
Member Author

EgorBo commented Jun 22, 2022

Are the fuzzlyn/antigen failures real?

Yes, I plan to address them tomorrow

@EgorBo EgorBo requested a review from MichalStrehovsky as a code owner June 30, 2022 17:16
@EgorBo
Copy link
Member Author

EgorBo commented Jul 1, 2022

/azp run fuzzlyn, antigen

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Jul 1, 2022

@AndyAyersMS PTAL, throughput regression is lowered from 1% to 0.01-0.1%, diff is slightly smaller but the pattern in question is handled. I plan a few more experiments here in future

Antigen/Fuzzlyn failures are unrelated

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Happy that you found a very surgical way to check what might be affected.

src/coreclr/jit/flowgraph.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/flowgraph.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/redundantbranchopts.cpp Outdated Show resolved Hide resolved
@EgorBo EgorBo merged commit 6b8d056 into dotnet:main Jul 2, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 1, 2022
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: optRedundantBranches misses an opportunity
3 participants