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.
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
Add memoization for const function evaluations #66294
Add memoization for const function evaluations #66294
Changes from 1 commit
5398139
a28fbd4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This means we do that even in the Miri interpreter, when such functions are run at "run-time", right? I think that's wrong: the function, even without any arguments, can access global state and cause aliasing violations on each call. So I think this PR as-is will regress Miri's ability to find UB.
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 propose to instead move memoization into const_eval's
find_fn
implementation.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 there a situation where a const fn can access global state which itself is not constant? This seems like the only situation to me where aliasing may be an issue? (But I think you know much more about Miri than me! 😄)
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.
How would you like such an implementation to work? Memoize the evaluation of the body and then return that from find_fn? Or memoize something else entirely?
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.
Well we might have to expand what
find_fn
can return, and the name might have to change reflecting that.Right now, probably not, but as
const fn
get more powerful and allowunsafe
code, that seems very possible to me. #66302 is a step in that direction. At that point CTFE relies on dynamic checks (at CTFE evaluation time), which Miri of course doesn't do as it runs run-time code.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.
Yikes - I wasn't aware we'll one day have unsafe code in const fn!
Ok. I'm going to be unavailable for the next week but will follow up on rust-lang/miri#1081 on my return.
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.
If we ever allowed (without it being UB) you to mutate or read mutable state in const fn, we couldn't ever do a MIR optimization that evals a const fn call where all args are known at compile-time. Also it would allow constants to read from global mutable state in its original state.
There's no need for dynamic checks if we do post monomorphization checks for this situation.
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.
We certainly shouldn't allow that during CTFE, but that's not what I talked about. I am talking about Miri-the-standalone-tool.
Now I am very confused, the dynamic checks are post-monomorphization? Both are the same thing to me in this context. Also I thought we all agreed that we should check everything dynamically so that even "Miri unleashed" is sound?
I thought we had consensus that the dynamic check described all the way at the bottom here should definitely be added. (Not that that is very relevant to this PR, but you brought it up.)
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.
Right, I don't remember what I meant by that.
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.
Does removing the use of
i32::CONSTANT
prevent this hard error? Previously, using#![allow(const_err)]
made this supressible.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.
Removing the use of
i32::CONSTANT
does indeed prevent the error. It would be nice if the error trace included mention ofi32::CONSTANT
so that users might figure this out.I've found a similar case on stable, which I've opened as a separate issue: #66332
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.
If the constant were public, the error would persist. So this is a breaking change, where a deny by default lint was changed into an error, but only if using the constant would have errored 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.
iirc there's a way to mark a query so that its cycles do something other than report a hard error, but I don't remember the details and whether it would help us here. I'll have a look tomorrow when I'm on a computer
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.
Great, thanks
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.
Sorry. This got lost in my todo list.
So We only have
cycle_delay_bug
, which is not really what we want here, because it will still cause the compilation to fail. We could specifically add acycle_future_incompat_lint
that works exactly likecycle_delay_bug
except that it emits a future incompat lint instead.cc @michaelwoerister I'm not sure if the query system can handle a query succeeding when a cycle occurs.
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.
Any cycle occurring should be fatal for the compilation session. I don't think we ever quite clarified if cycles can leave incr. comp. in an invalid state and sidestep the issue by never committing such possibly invalid state.
(cc @nikomatsakis, correct me if I'm wrong)
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.
Recovering from a query cycle is also problematic for a parallel compiler since the location we can recover from is not deterministic.
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.
Seems like the conclusion is that we will leave this cycle error as a failure. This is technically a breaking change (move from unusable-but-valid-compile to compile-fail). It's a weird edge case that didn't seem likely to be in use in the wild; indeed no regressions were found in the crater run.