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

Unmark loop align regardless if we found block to align or not #86198

Merged
merged 2 commits into from
May 16, 2023

Conversation

kunalspathak
Copy link
Member

When we place align instructions, there are few scenarios we check where we decide to not align the loop. However, we were incorrectly making that decision only if we don't find a block where we can place the align instruction. Instead, this should be checked regardless if we find such block or not.

For the problem reported in #85839, we were having loop block before loop top and we should have disabled loop alignment for such case, but were not doing it. Because of that we were not able to locate the back-edge of that loop leading to assert.

In case it helps to visualize, here is section of basic blocks:

BB197 [0329]  2       BB194,BB196          12.03      541 14 [2A0..2A1)-> BB199 ( cond )                     i bwd IBC LIR 
BB198 [0330]  1       BB197                12.03      541 14 [2A0..2A1)-> BB199 ( cond )                     i bwd bwd-src IBC LIR 
BB196 [0328]  1       BB198                11.50      517 14 [2A0..2A1)-> BB197 (always)                     i Loop Loop0 bwd bwd-target IBC LIR align 
BB160 [0036]  1       BB159                 2.22      100    [279..27A)-> BB161 ( cond )                     i IBC LIR 
BB162 [0303]  1       BB160                 2.22      100    [279..27A)-> BB165 ( cond )                     i IBC LIR 
BB164 [0308]  2       BB162,BB363          42.96     1933 10 [279..27A)-> BB165 ( cond )                     i Loop Loop0 nullcheck bwd bwd-target IBC LIR align 
BB363 [0532]  1       BB164                42.96          10 [???..???)-> BB164 (always)                     internal LIR 

The loop that starts at BB196 had the loop block BB197 before the loop top block (BB196).

Mapped BB196 to G_M24672_IG114
Marking BB134 that ends with unconditional jump with BBF_HAS_ALIGN for loop at BB196

BB197 [0329]  2       BB194,BB196          12.03      541 14 [2A0..2A1)-> BB199 ( cond )                     i bwd IBC LIR 
BB198 [0330]  1       BB197                12.03      541 14 [2A0..2A1)-> BB199 ( cond )                     i bwd bwd-src IBC LIR 
BB196 [0328]  1       BB198                11.50      517 14 [2A0..2A1)-> BB197 (always)                     i Loop Loop0 bwd bwd-target IBC LIR align 

Because of this, we don't find the back-edge for IG114 as seen in the assert:

Mismatch in align instruction.
Containing IG: IG98
loopHeadPredIG: IG113
loopHeadIG: IG114
igInLoop: IG116
igInLoop->backEdge: IG116
igInLoop has align instruction for : IG117
Loop:
	IG114
	IG115
	IG116
	IG117
        ...

Thanks @AndyAyersMS for the repro. It was very quick to spot the problem.

Fixes: #85839

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

ghost commented May 13, 2023

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

Issue Details

When we place align instructions, there are few scenarios we check where we decide to not align the loop. However, we were incorrectly making that decision only if we don't find a block where we can place the align instruction. Instead, this should be checked regardless if we find such block or not.

For the problem reported in #85839, we were having loop block before loop top and we should have disabled loop alignment for such case, but were not doing it. Because of that we were not able to locate the back-edge of that loop leading to assert.

In case it helps to visualize, here is section of basic blocks:

BB197 [0329]  2       BB194,BB196          12.03      541 14 [2A0..2A1)-> BB199 ( cond )                     i bwd IBC LIR 
BB198 [0330]  1       BB197                12.03      541 14 [2A0..2A1)-> BB199 ( cond )                     i bwd bwd-src IBC LIR 
BB196 [0328]  1       BB198                11.50      517 14 [2A0..2A1)-> BB197 (always)                     i Loop Loop0 bwd bwd-target IBC LIR align 
BB160 [0036]  1       BB159                 2.22      100    [279..27A)-> BB161 ( cond )                     i IBC LIR 
BB162 [0303]  1       BB160                 2.22      100    [279..27A)-> BB165 ( cond )                     i IBC LIR 
BB164 [0308]  2       BB162,BB363          42.96     1933 10 [279..27A)-> BB165 ( cond )                     i Loop Loop0 nullcheck bwd bwd-target IBC LIR align 
BB363 [0532]  1       BB164                42.96          10 [???..???)-> BB164 (always)                     internal LIR 

The loop that starts at BB196 had the loop block BB197 before the loop top block (BB196).

Mapped BB196 to G_M24672_IG114
Marking BB134 that ends with unconditional jump with BBF_HAS_ALIGN for loop at BB196

BB197 [0329]  2       BB194,BB196          12.03      541 14 [2A0..2A1)-> BB199 ( cond )                     i bwd IBC LIR 
BB198 [0330]  1       BB197                12.03      541 14 [2A0..2A1)-> BB199 ( cond )                     i bwd bwd-src IBC LIR 
BB196 [0328]  1       BB198                11.50      517 14 [2A0..2A1)-> BB197 (always)                     i Loop Loop0 bwd bwd-target IBC LIR align 

Because of this, we don't find the back-edge for IG114 as seen in the assert:

Mismatch in align instruction.
Containing IG: IG98
loopHeadPredIG: IG113
loopHeadIG: IG114
igInLoop: IG116
igInLoop->backEdge: IG116
igInLoop has align instruction for : IG117
Loop:
	IG114
	IG115
	IG116
	IG117
        ...

Thanks @AndyAyersMS for the repro. It was very quick to spot the problem.

Fixes: #85839

Author: kunalspathak
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@kunalspathak
Copy link
Member Author

/azp run runtime-coreclr libraries-pgo

@kunalspathak kunalspathak marked this pull request as ready for review May 15, 2023 16:17
@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kunalspathak
Copy link
Member Author

Failures is from #85233

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.

Test failure: Mismatch in align instruction
2 participants