-
Notifications
You must be signed in to change notification settings - Fork 12.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
Implement tainted_by_errors
in MIR borrowck, use it to skip CTFE
#93691
Merged
+255
−213
Merged
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
4ad272b
implement tainted_by_errors in mir borrowck
compiler-errors 77dae2d
skip const eval if we have an error in borrowck
compiler-errors 06eb912
fix tests, add new tests checking borrowck CFTE ICE
compiler-errors 8b7b0a0
always cache result from mir_borrowck
compiler-errors 29c2bb5
rework borrowck errors so that it's harder to not set tainted
compiler-errors a431174
add tainted_by_errors to mir::Body
compiler-errors 67ad0ff
use body.tainted_by_error to skip loading MIR
compiler-errors File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
always cache result from mir_borrowck
commit 8b7b0a0e49cf3e313d0432d1f6e8ce20253fef0a
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 is this necessary?
I'm going to do a perf run,? but maybe we just need to check the tainted flag here, too
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 was getting ICEs in incremental tests complaining that the MIR body for a fn was stolen, since we're now calling borrowck which depends on unoptimized MIR which is probably gone if we do const eval late in the compilation process...
Not sure if only caching if the body if it is tainted works, since we're unconditionally calling borrowck before const eval.
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.
heh, yea, this is what @BoxyUwU ran into, too, and we never got as far as to figuring it out.
I guess it makes sense that if the cache rules differ between two queries that work on stealing things, that said caching breaks, but it's really opaque to me. Well... let's see what perf says
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.
Perf does show the additional query result serialization, so we should try to do something 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's probably not worth it, but we could try adding a query that returns just an
Option<ErrorReported>
and does all the processing of typeck, borrowck and const qualifs. Then we can only cache that on disk and it should rarely get recomputed. But: new query so probably overkill, but worth an experiment?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.
Yeah, I'm not exactly sure what we can do to limit
cache_on_disk
just to when it's needed, since it seems like we need to cache it for any body that has a promoted const, see the failing query stack trace:... but we don't know if a body has any promoted consts until after borrowck.
so I might try the query experiment you suggested. I'll ping you for a perf run when I put it up, since I don't have super powers.
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.
Actually, we do know it at borrowck time, so we could encode that info in the borrowck result.
But maybe we're going about this the wrong way. When we force mir borrowck and then steal the mir, we own the mir, so we can mutate it and change its tainted field. This is essentially the same idea as the merging of the tainted flags into one query only we're putting it right in the data we have to load anyway
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.
Oh lol, I thought we did borrowck on
mir_built
, whoopsy.So you're suggesting we do add the tainted field to
mir::Body
then, instead ofBorrowCheckerResult
?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.
You need it on both, as (as you reminded me) you can't mutate the mir from borrowck. But at the query that steals the mir after forcing the borrowck query, you can just call borrowck instead of forcing it and copy the tainted field over to the mir body you just stole