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

JIT: make global morph its own phase #94185

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

AndyAyersMS
Copy link
Member

Contributes to #93246.

@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 Oct 30, 2023
@ghost ghost assigned AndyAyersMS Oct 30, 2023
@ghost
Copy link

ghost commented Oct 30, 2023

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

Issue Details

Contributes to #93246.

Author: AndyAyersMS
Assignees: AndyAyersMS
Labels:

area-CodeGen-coreclr

Milestone: -

@AndyAyersMS
Copy link
Member Author

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.

@AndyAyersMS
Copy link
Member Author

No diffs, errors known.

Comment on lines +13895 to +13897
// 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.
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's fine.

@AndyAyersMS AndyAyersMS merged commit a7b0249 into dotnet:main Oct 31, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Nov 30, 2023
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.

2 participants