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

Fix perf regressions with a recent change. #55300

Merged
merged 3 commits into from
Jul 8, 2021

Conversation

sandreenko
Copy link
Contributor

#54998 introduced a perf regression that this PR is fixing.

Thanks to @kunalspathak for catching and @EgorBo for repro help.

The regression was happening only when we switched from MinOpts to FullOpts and it was happening because of Tier0 QuickJitForLoops=disabled, so SPMI did not reproduce this scenario so it was not catched in my local testing.

We still want to have doNotEnreg flag to be known as soon as possible but with this change, we just add two additional iterations to set it for minopts in comparasing with the previous version that was trying to init it when we created a lclVar.

Why can't we init it when we create a lclVar? It is because at this time we don't know the final optimization level yet and we can't reset doNotEnreg flag after the optimization flag is finalized.

No SPMI diffs as expected but CoreRun with Tiering=1 shows improvements on methods with loops.

@sandreenko sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 7, 2021
@sandreenko
Copy link
Contributor Author

PTAL @kunalspathak @EgorBo @dotnet/jit-contrib

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

Did you try running some BDN benchmarks that regressed and were able to see the gains come back?

{
// Morph is checking if lvDoNotEnregister is already set for some optimizations.
// If we are running in min opts and know that we won't enregister any locals
// it is better to set this flag before we start reading it.
Copy link
Member

Choose a reason for hiding this comment

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

I was hoping to see MinOpts check here...why is it not there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment was poorly written, what I was trying to explain here:

  1. the main flag is (opts.compFlags & CLFLG_REGVAR), if it is not set we can't enregister variables;

  2. right now it is set if we use CLFLG_MAXOPT and not set when using CLFLG_MINOPT, an example where we set it:

    // Don't optimize .cctors (except prejit) or if we're an inlinee
    else if (!jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT) && ((info.compFlags & FLG_CCTOR) == FLG_CCTOR) &&
    !compIsForInlining())
    {
    opts.compFlags = CLFLG_MINOPT;
    }

  3. in theory nothing prevents us to disable CLFLG_REGVAR when MinOpts is not set, or enable it when it is set, so I do not want to check isMinOpts instead of !compEnregLocals() here.

I have updated the comments to reflect that, please take a look.

@sandreenko
Copy link
Contributor Author

Did you try running some BDN benchmarks that regressed and were able to see the gains come back?

Yes, they are back to the previous values.

@sandreenko
Copy link
Contributor Author

the failures are unrelated.

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM

Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
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.

2 participants