Check if loop body occured before loopTop and if so unmark alignment #91854
Conversation
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsWith optimizations conducted in various phases, we might rearrange the blocks such that loop body appears lexically before the loopTop. As such, we should unmark the alignment of such loops, because when we try to find the size of the loop, we don't find the block that targets back to the loop top. We were previously unmarking such loops, however, it was only if the loop body appears just before the loop top. In the failure case, the loop top is several blocks below the loop body. Updated the code in Fixes: #91838
|
|
@dotnet/jit-contrib @AndyAyersMS |
|
/backport to release/8.0 |
|
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6154847476 |
| @@ -5276,6 +5276,13 @@ PhaseStatus Compiler::placeLoopAlignInstructions() | |||
| weight_t minBlockSoFar = BB_MAX_WEIGHT; | |||
| BasicBlock* bbHavingAlign = nullptr; | |||
| BasicBlock::loopNumber currentAlignedLoopNum = BasicBlock::NOT_IN_LOOP; | |||
| bool visitedLoopNum[BasicBlock::MAX_LOOP_NUM]; | |||
| memset(visitedLoopNum, false, sizeof(visitedLoopNum)); | |||
There was a problem hiding this comment.
It's weird to use memset to zero a bool array, but ok.
|
|
||
| #ifdef DEBUG | ||
| int visitedBlockForLoopNum[BasicBlock::MAX_LOOP_NUM]; | ||
| memset(visitedBlockForLoopNum, false, sizeof(visitedBlockForLoopNum)); |
There was a problem hiding this comment.
It's weird to use false as a value to zero an int array.
| memset(visitedLoopNum, false, sizeof(visitedLoopNum)); | ||
|
|
||
| #ifdef DEBUG | ||
| int visitedBlockForLoopNum[BasicBlock::MAX_LOOP_NUM]; |
There was a problem hiding this comment.
block nums can't be negative; use unsigned?
| @@ -5352,12 +5359,19 @@ PhaseStatus Compiler::placeLoopAlignInstructions() | |||
| madeChanges = true; | |||
| unmarkedLoopAlign = true; | |||
| } | |||
| else if ((block->bbNatLoopNum != BasicBlock::NOT_IN_LOOP) && (block->bbNatLoopNum == loopTop->bbNatLoopNum)) | |||
| else if (visitedLoopNum[loopTop->bbNatLoopNum]) | |||
There was a problem hiding this comment.
Don't you need to check that block->bbNatLoopNum != BasicBlock::NOT_IN_LOOP?
There was a problem hiding this comment.
Not really. block is the previous block before loopTop and we want to check here if we have seen a loop body related to loopTop->bbNatLoopNum.
There was a problem hiding this comment.
Oh, sorry, I meant, don't you need to check loopTop->bbNatLoopNum != BasicBlock::NOT_IN_LOOP?
It appears that sometimes it does equal NOT_IN_LOOP, leading to reading beyond the array bounds.
ghost
locked as resolved and limited conversation to collaborators
With optimizations conducted in various phases, we might rearrange the blocks such that loop body appears lexically before the loopTop. As such, we should unmark the alignment of such loops, because when we try to find the size of the loop, we don't find the block that targets back to the loop top. We were previously unmarking such loops, however, it was only if the loop body appears just before the loop top. In the failure case, the loop top is several blocks below the loop body. Updated the code in
placeLoopAlignInstructionsto track if we have already seen a particularbbNatLoopNum. If yes, and a block is marked as aligned, that means we have already encountered the loop body block that had the samebbNatLoopNum. Unmark such loops.Fixes: #91838