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

Do not compact blocks around loops #71868

Merged
merged 1 commit into from
Jul 10, 2022

Conversation

kunalspathak
Copy link
Member

We would sometimes compact two blocks where 2nd block participates needs loop alignment. If the 1st block (in which 2nd block was merged) was part of an outer loop, then we would falsely think that we have seen a loop block before the top block and disable aligning that loop. That introduces regression in some benchmarks.

The fix would be to not compact such blocks. Also while working, I noticed that we also compact blocks that are part of different loops and that might make bbNatLoopNum information outdated. fgUpdateFlowGraph() (which calls the block compaction code) is called multiple times from various phases and it will be good if we have the bbNatLoopNum information as accurate as possible. Hence, I also added a check that will prohibit us from compacting such blocks that are part of different loops.

From this PR, we get back the alignment in the benchmark.

image

Relevant discussion: #71646 (comment)

Fixes: #71646

@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 9, 2022
@ghost ghost assigned kunalspathak Jul 9, 2022
@ghost
Copy link

ghost commented Jul 9, 2022

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

Issue Details

We would sometimes compact two blocks where 2nd block participates needs loop alignment. If the 1st block (in which 2nd block was merged) was part of an outer loop, then we would falsely think that we have seen a loop block before the top block and disable aligning that loop. That introduces regression in some benchmarks.

The fix would be to not compact such blocks. Also while working, I noticed that we also compact blocks that are part of different loops and that might make bbNatLoopNum information outdated. fgUpdateFlowGraph() (which calls the block compaction code) is called multiple times from various phases and it will be good if we have the bbNatLoopNum information as accurate as possible. Hence, I also added a check that will prohibit us from compacting such blocks that are part of different loops.

From this PR, we get back the alignment in the benchmark.

image

Relevant discussion: #71646 (comment)

Fixes: #71646

Author: kunalspathak
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib

@kunalspathak kunalspathak merged commit 26ec70b into dotnet:main Jul 10, 2022
@BruceForstall
Copy link
Member

Why can't:

if ((block->bbNatLoopNum != BasicBlock::NOT_IN_LOOP) && (bNext->bbNatLoopNum != BasicBlock::NOT_IN_LOOP) &&
        (block->bbNatLoopNum != bNext->bbNatLoopNum))

be:

if (block->bbNatLoopNum != bNext->bbNatLoopNum)

?

@EgorBo
Copy link
Member

EgorBo commented Jul 14, 2022

Improvements on Linux-arm64: dotnet/perf-autofiling-issues#6774

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.

Regressions from loop alignment fixes
4 participants