Skip to content

Reset loop numbers in RecomputeLoopInfo #56981

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

Merged

Conversation

SingleAccretion
Copy link
Contributor

They become stale after optimizations.

I will note that the problem fgDebugCheckLoopTable here is "real", and I've investigated enabling fgDebugCheckLoopTable after the unroller phase, but it turns out not to be so trivial.

Take the following, for example:

for (int i = 0; i < 1; i++)
{
    for (int y = 0; y < 10; y++)
    {
        sum += *b;
    }
}

We will unroll the outer loop, but will not reset the loop number on any blocks, ending up with the following flow table:

-----------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight    lp [IL range]     [jump]      [EH region]         [flags]
-----------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1                             1       [000..006)                                     i
BB02 [0001]  2       BB01,BB04             4     0 [006..00A)                                     i bwd bwd-target
BB03 [0002]  2       BB02,BB03            16     1 [00A..018)                                     i bwd bwd-target
BB04 [0004]  2       BB02,BB03             4     0 [018..020)                                     i bwd
BB05 [0009]  2                             0.50  0 [006..00A)-> BB07 ( cond )                     i Loop bwd bwd-target
BB06 [0010]  2                             2     1 [00A..018)-> BB06 ( cond )                     i Loop bwd bwd-target
BB07 [0011]  2                             0.50  0 [018..020)                                     i bwd
BB08 [0006]  2       BB01,BB04             1       [020..022)        (return)                     i
-----------------------------------------------------------------------------------------------------------------------------------------

Note the loop numbers above are quite incorrect. The loop info for L1 now references empty blocks that the unroller made invalid, and we have "two" loops for the same number. The naive fix would be to invalidate all loops (including the inner ones) during unrolling, but such a fix results in a big (25%+) PerfScore regression in BenchI.Puzzle:DoIt, so I punted it. A proper fix would involve reusing the existing blocks for the first (or whichever) iteration while unrolling.

Fixes #56961.

They become stale after optimizations.
@ghost ghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member labels Aug 6, 2021
@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Aug 6, 2021
@JulieLeeMSFT
Copy link
Member

@kunalspathak PTAL.

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.

Thanks for fixing this!

@kunalspathak kunalspathak merged commit b6647fc into dotnet:main Aug 6, 2021
@AndyAyersMS
Copy link
Member

Did we look at diffs?

@SingleAccretion
Copy link
Contributor Author

Since we don't test this configuration often it seems, getting diffs might be challenging.

But I will see what can be done.

@AndyAyersMS
Copy link
Member

I just mean checking normal diffs.

If we're confident RecomputeLoopInfo is only reachable with COMPlus_JitOptRepeat=* then don't bother.

@SingleAccretion
Copy link
Contributor Author

If we're confident RecomputeLoopInfo is only reachable with COMPlus_JitOptRepeat=*

That is indeed the case:

void Compiler::RecomputeLoopInfo()
{
assert(opts.optRepeat);
assert(JitConfig.JitOptRepeatCount() > 0);

@SingleAccretion SingleAccretion deleted the Clear-NatNum-In-Opt-Repeat branch August 10, 2021 20:04
@ghost ghost locked as resolved and limited conversation to collaborators Sep 9, 2021
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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'block->bbNatLoopNum == BasicBlock::NOT_IN_LOOP' in 'System.SpanHelpers:LastIndexOf(byref,ushort,int):int' during 'Optimize index checks'
4 participants