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

Force new label after IG that ends with align instruction #62025

Merged
merged 1 commit into from
Dec 1, 2021

Conversation

kunalspathak
Copy link
Member

@kunalspathak kunalspathak commented Nov 24, 2021

Sometimes, we might end up having 2 jmp instructions as part of same block and we might prefer to add align instruction after the 1st jump but then we would assert that there is no label present. I can update the assert condition to something like below, but I think proper fix would be to set needLabel = true in such cases.

assert(needLabel ||  ((block->bbPrev->bbJumpKind == BBJ_ALWAYS) && (block->bbJumpKind == BBJ_ALWAYS)));
       E97AFFFFFF           jmp      G_M22669_IG12
       E9A0060000           jmp      G_M22669_IG78

Fixes: #61939

@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 Nov 24, 2021
@ghost
Copy link

ghost commented Nov 24, 2021

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

Issue Details

Sometimes, we might end up having 2 jmp instructions as part of same block and we might prefer to add align instruction after the 1st jump but then we would assert that there is no label present. I can update the assert condition to something like below, but I think proper fix would be to set needLabel = true in such cases.

assert(needLabel ||  ((block->bbPrev->bbJumpKind == BBJ_ALWAYS) && (block->bbJumpKind == BBJ_ALWAYS)));
       E97AFFFFFF           jmp      G_M22669_IG12
       E9A0060000           jmp      G_M22669_IG78
Author: kunalspathak
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib @BruceForstall

@kunalspathak kunalspathak changed the title needLabel assert Force new label after IG that ends with align instruction Nov 29, 2021
@BruceForstall
Copy link
Member

@kunalspathak Can you explain how we can get back-to-back jmp in the same IG? Wouldn't that make the second one dead code?

@kunalspathak
Copy link
Member Author

That magic happens when we decide to "Conditionally fold" a basic block, but while doing that, do not check if previous block also had an unconditional jump.

Initial flow:

BB16  [0101]  1  0    BB15                  0.90 706    [06A..06B)-> BB22  ( cond ) T0                  keep i nullcheck IBC 
BB17  [0105]  1  0    BB16                  0.90 706    [06A..06B)-> BB22  ( cond ) T0                  i IBC 
BB18  [0106]  1  0    BB17                  0.77 607    [06A..06B)-> BB22  ( cond ) T0                  i IBC 
BB19  [0107]  1  0    BB18                  0.72 569    [06A..06B)-> BB24  ( cond ) T0                  i IBC 
BB20  [0298]  1  0    BB19                  0.72 569    [???..???)-> BB110 (always) T0                  internal IBC 
BB21  [0091]  1  0    BB13                  0.12  98    [04A..04B)-> BB14  (always) T0                  i hascall gcsafe idxlen IBC 
BB22  [0108]  3  0    BB16,BB17,BB18        0.17 137    [06A..06B)                  T0                  i IBC 
BB23  [0110]  1  0    BB22                  0.90 706    [???..???)-> BB110 ( cond ) T0                  keep internal nullcheck IBC 

During conditional folding of jumps, we do this:

Conditional folded at BB23
BB23 becomes a BBJ_ALWAYS to BB110

which transforms the flow to:

BB21  [0091]  1  0    BB13                  0.12  98    [04A..04B)-> BB14  (always) T0                  i hascall gcsafe idxlen IBC 
BB22  [0108]  3  0    BB16,BB17,BB18        0.17 137    [06A..06B)                  T0                  i IBC 
BB23  [0110]  1  0    BB22                  0.90 706    [???..???)-> BB110 (always) T0                  keep internal nullcheck IBC 
BB24  [0015]  3  0    BB19,BB110,BB114      0.71 563    [190..19B)-> BB29  ( cond ) T0                  i nullcheck IBC 
BB25  [0118]  1  0    BB24                  0.32 251    [190..191)-> BB29  ( cond ) T0                  i IBC 

We also optimize and remove BB22 and redirect BB16, BB17 and BB18 to BB110 as part of "Optimizing a jump to an unconditional jump". So BB22 is not left with any predecessor and technically dead.

During codegen, we get this:

BB16  [0101]  1  0    BB15                  0.90 706    [06A..06B)-> BB110 ( cond ) T0                  keep i nullcheck IBC LIR 
BB17  [0105]  1  0    BB16                  0.90 706    [06A..06B)-> BB110 ( cond ) T0                  i IBC LIR 
BB18  [0106]  1  0    BB17                  0.77 607    [06A..06B)-> BB110 ( cond ) T0                  i IBC LIR 
BB19  [0107]  1  0    BB18                  0.72 569    [06A..06B)-> BB24  ( cond ) T0                  i IBC LIR 
BB20  [0298]  1  0    BB19                  0.72 569    [???..???)-> BB110 (always) T0                  internal IBC LIR 
BB165 [0315]  0  0                          0           [???..???)         (throw ) T0                  keep i internal rare label LIR 
BB21  [0091]  1  0    BB13                  0.12  98    [04A..04B)-> BB14  (always) T0                  i label hascall gcsafe idxlen IBC LIR 
BB23  [0110]  0  0                          0.90 706    [???..???)-> BB110 (always) T0                  keep internal nullcheck IBC LIR 

So yes, the second jmp is dead code that stays because of existing shortcoming.

@kunalspathak
Copy link
Member Author

jitstress2-jitstressregs failures are related to #61899
jitstress failures are related to #60330 and #62238

@kunalspathak kunalspathak merged commit 7ffc96e into dotnet:main Dec 1, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 31, 2021
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.

Number of jit tests failing with needLabel assert
2 participants