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

Loop alignment: Fix loop size calculation to exclude IG's size that are marked as not aligned #68869

Merged
merged 8 commits into from
May 11, 2022

Conversation

kunalspathak
Copy link
Member

@kunalspathak kunalspathak commented May 4, 2022

As explained in #68510 (comment), there is a scenario when setting the loop back edge that we know we cannot align a loop and removes the IGF_LOOP_ALIGN flag without fixing the igSize. That affects the loopSize calculation depending on if it was done during loop alignment adjustment or during output.

Edit:

I have added a flag IGF_REMOVED_ALIGN but commented out the usage for now. I have added an assert to see if this was
silently missed (because we would not align a loop for some heuristic reason). I will enable the usage of IGF_REMOVED_ALIGN after that.

There were test failures https://dev.azure.com/dnceng/public/_build/results?buildId=1752445&view=results so definitely we
were hitting this scenario, but it was not failing because the loop might be big enough to not align. I have figured out another
way in 3704be5 to track the information of IGs whose align flag was removed instead of
adding IGF_REMOVED_ALIGN.

#65690: In this case, we had 2 backedges of a loop that was marked for aligned. When we set the 1st backedge, we realize that it is enclosing a loop that is marked for alignment, so we unset the flag of current loop. We visit 2nd backedge and again try to unset the align flag and expect that it was not already unset. I have updated the assert using removedAlignFlag to say that either the align flag should be present, or it was present at one point and then we removed it.

#65988: We decide to not place align instruction in prolog, but we didn't remove the align flag which later causes problem. 2322a67 fixes that.

Fixes: #68510, #65690 and #65988

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

ghost commented May 4, 2022

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

Issue Details

As explained in #68510 (comment), there is a scenario when setting the loop back edge that we know we cannot align a loop and removes the IGF_LOOP_ALIGN flag without fixing the igSize. That affects the loopSize calculation depending on if it was done during loop alignment adjustment or during output.

I have added a flag IGF_REMOVED_ALIGN but commented out the usage for now. I have added an assert to see if this was silently missed (because we would not align a loop for some heuristic reason). I will enable the usage of IGF_REMOVED_ALIGN after that.

Fixes: #68510

Author: kunalspathak
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

src/coreclr/jit/emit.cpp Outdated Show resolved Hide resolved
Co-authored-by: Wraith <wraith2@gmail.com>
@kunalspathak
Copy link
Member Author

/azp run runtime-coreclr jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kunalspathak
Copy link
Member Author

So, we have several tests that hits the assert I added for testing which means that even today, we mismatch the actual vs. expected padding because of resolution blocks being added in between the loop making bbNatLoopNum unreliable.

@kunalspathak kunalspathak changed the title Flag to track IGs whose alignment was removed Loop alignment: Fix loop size calculation to exclude IG's size that are marked as not aligned May 5, 2022
@kunalspathak kunalspathak marked this pull request as ready for review May 5, 2022 21:11
@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib , @BruceForstall

src/coreclr/jit/compiler.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/emit.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/emit.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/emit.cpp Outdated Show resolved Hide resolved
@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels May 8, 2022
src/coreclr/jit/clrjit.natvis Outdated Show resolved Hide resolved
src/coreclr/jit/emit.h Outdated Show resolved Hide resolved
@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels May 10, 2022
jakobbotsch added a commit that referenced this pull request Jun 2, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 11, 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
4 participants