-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
JIT: make global morph its own phase #94185
Conversation
Contributes to dotnet#93246.
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsContributes to #93246.
|
PTAL @dotnet/jit-contrib Once RPO is in place, the block traversal order will change, so split out the per-block action as its own method. No diffs. Has bugged me forever that there was no default IR dump after morph. Now there is. |
No diffs, errors known. |
// Note morph can add blocks downstream from the current block, | ||
// and alter (but not null out) the current block's bbNext; | ||
// this iterator ensures they all get visited. |
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.
Doesn't the iterator load the bbNext before the iteration of the loop? If the callee can adjust the blocks, maybe we shouldn't use the for-based iterator here?
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.
It doesn't seem to cache anything...? Also that comment is a bit stale; with other changes I've made recently morph won't add blocks or (I suspect) alter bbNext
anymore.
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.
Maybe it only caches the end-point of the iteration. But still, any change to the block list seems risky to depend on what the C++ compiler is going to do.
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.
For the BasicBlockRangeList
form sure... but this is going via BasicBlockSimpleList
which does't cache an endpoint.
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.
@BruceForstall still ok with merging? Or would you rather I change the iteration back?
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.
Yes, it's fine.
Contributes to #93246.