-
Notifications
You must be signed in to change notification settings - Fork 5.3k
JIT: Update terminology in loop cloning, and simplify a bit #95648
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
Conversation
Update some terminology in loop cloning; rewrite docs to be less "lexical" oriented; move responsibility of maintenance of old preheader -> first cond link out of `CondToStmtInBlock`.
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsUpdate some terminology in loop cloning; rewrite docs to be less "lexical" oriented; move maintenance of old preheader -> first cond link out of No diffs expected.
|
src/coreclr/jit/loopcloning.cpp
Outdated
| // Make 'h' fall through to 'h2' (if it didn't already). | ||
| // Don't add the h->h2 edge because we're going to insert the cloning conditions between 'h' and 'h2', and | ||
| // optInsertLoopChoiceConditions() will add the edge. | ||
| h->SetJumpDest(h2); | ||
| assert(h->JumpsToNext()); | ||
| h->bbFlags |= BBF_NONE_QUIRK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this part was a bit odd -- we make h (now preheader) jump to h2 (now fastPreheader), then insert conditions between preheader and fastPreheader and make preheader jump to the first condition instead (which is done inside CondToStmtInBlock). It seemed like CondToStmtInBlock shouldn't be responsible for updating this link, so I've updated it so that we just leave preheader alone until the end of this function and then directly set it to jump to the first choice condition.
|
cc @dotnet/jit-contrib PTAL @BruceForstall x64 superpmi-replay failure is the timeout we see once in a while. x86 failure is presumably the one fixed by #95671. |
BruceForstall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
src/coreclr/jit/loopcloning.cpp
Outdated
| // the loop being cloned. | ||
| unsigned char ambientLoop = m_newToOldLoop[loop->GetIndex()]->lpParent; | ||
|
|
||
| BasicBlock* h = preheader; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is h still used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, will remove it
|
This is going to conflict with #95139. Can you wait for that one? |
Sure, looks like it was just merged. |
|
Closing and reopening to pick up #95718 |
Update some terminology in loop cloning; rewrite docs to be less "lexical" oriented; move maintenance of old preheader -> first cond link out of
CondToStmtInBlock.No diffs expected.