-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Region inference: split results from RegionInferenceContext #151688
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
base: main
Are you sure you want to change the base?
Region inference: split results from RegionInferenceContext #151688
Conversation
This comment has been minimized.
This comment has been minimized.
6befd69 to
13a57f6
Compare
This comment has been minimized.
This comment has been minimized.
13a57f6 to
00e8121
Compare
| @@ -618,7 +624,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { | |||
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.
why split out the scc_values but not the scc graph itself?
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.
That would require a larger change to RegionInferenceContext, since it uses it for almost everything. I could try though and see where that goes.
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 agree it would be more elegant
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 mean, the best approach would probably be that RegionInferenceContext borrows a bunch of stuff, does its computation and then dies once inference is complete, and the results are what's stored.
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.
Update: that is more elegant, but it also requires moving the SCCs out of RegionInferenceContext, which collapses like a soufflé and the change becomes enormous. I'm trying that, because I suspect the more decentralised architecture might help with Polonius not recomputing things in the future, but it turns out that everyone kind of expect regioncx to be around absolutely everywhere and use it to pilfer various data they need, usually not many things.
The good news are that data dependencies are now much easier to see for humans (and maybe also the compiler).
This ensures all of region inference is immutable, and makes every operation that requires region inference to have been executed to run explicitly require the results.
Since this collapses `RegionInferenceContext`, `::solve()` now consumes the inference context.
00e8121 to
454f98a
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
This ensures all of region inference is immutable, and makes every operation that requires region inference to have been executed to run explicitly require the results.
I have chosen to newtype the results and lightly abstract it from outside viewers, since it's required by some consumers of the compiler internal APIs. Those should be able to find what they need to exercise the features they used to need.
r? @lcnr