Skip to content

Print the state when the align instructions don't match up#83286

Merged
kunalspathak merged 7 commits intodotnet:mainfrom
kunalspathak:loop_align
Mar 20, 2023
Merged

Print the state when the align instructions don't match up#83286
kunalspathak merged 7 commits intodotnet:mainfrom
kunalspathak:loop_align

Conversation

@kunalspathak
Copy link
Contributor

@kunalspathak kunalspathak commented Mar 10, 2023

In #82729, we hit a problem where we have align instruction for a loop that also contains an align instruction for a different loop. This state is not permitted. So, print the state of loops with relevant information. Most likely, the fix for such cases would be in placeLoopAlignInstructions() but it is hard to determine the missing case without a repro. This was happening because during block compaction, we were failing to update the bbNatLoopNum of a block A into which the other block B is getting compacted into. Block B turned out to be backward jump source of a loop. Block A was placed lexically before the top-loop. In placeLoopAlignInstructions(), we handle such situations where if we see a loop body before the loop-top, we disregard such loop from getting aligned, but since bbNatLoopNum was not up to date, we were failing to recognize it. With the fix, we will unmark such loops from participating in alignment.

Fixes: #82729

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 10, 2023
@ghost ghost assigned kunalspathak Mar 10, 2023
@ghost
Copy link

ghost commented Mar 10, 2023

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

Issue Details

In #82729, we hit a problem where we have align instruction for a loop that also contains an align instruction for a different loop. This state is not permitted. So, print the state of loops with relevant information. Most likely, the fix for such cases would be in placeLoopAlignInstructions() but it is hard to determine the missing case without a repro.

Author: kunalspathak
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@kunalspathak
Copy link
Contributor Author

@dotnet/jit-contrib

@AndyAyersMS
Copy link
Member

Didn't we recently make some change to SPMI to hide diffs from alignment changes? Seems like you might want to run asmdiffs locally with this disabled to be sure nothing unexpected is happening.

@kunalspathak
Copy link
Contributor Author

yes, I forgot about it. pushed.

@kunalspathak kunalspathak added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 17, 2023
@kunalspathak
Copy link
Contributor Author

yes, I forgot about it. pushed.

No diffs.

This reverts commit c10b70d.
@kunalspathak kunalspathak removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 18, 2023
block->bbNatLoopNum, bNext->bbNatLoopNum);
block->bbNatLoopNum = bNext->bbNatLoopNum;
}

Copy link
Member

Choose a reason for hiding this comment

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

Should we also update the loop table -- presumably this loop now has a new top? Or can this only happen when we no longer are maintaining the loop table?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do update loop table by calling fgUpdateLoopsAfterCompacting() below.

@kunalspathak kunalspathak merged commit a23cfd3 into dotnet:main Mar 20, 2023
@kunalspathak kunalspathak deleted the loop_align branch March 20, 2023 17:58
@ghost ghost locked as resolved and limited conversation to collaborators Apr 19, 2023
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.

Assertion failed '(igInLoop->igLoopBackEdge == nullptr) || (igInLoop->igLoopBackEdge == igLoopHeader)'

2 participants