Add memoization for const function evaluations #66294
Merged
Conversation
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
rust-highfive
added
the
S-waiting-on-review
|
@bors try |
bors
added a commit
that referenced
this pull request
Nov 11, 2019
Add memoization for const function evaluations When a const function is being evaluated, as long as all its arguments are zero-sized-types (or it has no arguments) then we can trivially memoize the evaluation result using the existing query mechanism. With thanks to @oli-obk for mentoring me through this at RustFest Barcelona. r? @oli-obk
|
@rust-timer build 29354cb |
|
Queued 29354cb with parent 9248b01, future comparison URL. |
Centril
reviewed
Nov 11, 2019
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☀️ Try build successful - checks-azure |
|
Finished benchmarking try commit 29354cb, comparison URL. |
|
We need to think about how this interacts with const heap. If a const fn computes a const heap value, it will now be deduplicated. So if we initialize two statics with such a function, they would refer to the same const heap value. |
|
This is insane: |
|
We may want to alter that benchmark such that it's not mostly testing the no-argument case -- otherwise all of the repeat calls are just cached and it's no longer quite benchmarking what we want it to, probably :) |
I was actually talking about exactly this to @oli-obk this morning! I'll submit a PR to https://github.com/rust-lang-nursery/rustc-perf shortly ;) |
bjorn3
reviewed
Nov 12, 2019
|
|
||
| For more information about this error, try `rustc --explain E0080`. | ||
| For more information about this error, try `rustc --explain E0391`. |
There was a problem hiding this comment.
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.
Removing the use of i32::CONSTANT does indeed prevent the error. It would be nice if the error trace included mention of i32::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.
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.
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.
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 a cycle_future_incompat_lint that works exactly like cycle_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.
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.
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.
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.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
davidhewitt
commented
Nov 12, 2019
oli-obk
reviewed
Nov 12, 2019
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #60026) made this pull request unmergeable. Please resolve the merge conflicts. |
davidhewitt
force-pushed
the
const_fn_memoization
branch
from
40cea80
to
eacf695
Compare
|
Have rebased and squashed. On a laptop without mains power at the moment so I haven't run the test suite lest I kill the machine building llvm 😆 WRT this comment:
I chatted with @oli-obk in-person a little more about this. As I understand it, at the moment we haven't implemented any const heap, so there is currently no clash with this optimisation. When const heap does land, we'll need to ensure that there's no soundness issues caused by this optimisation. If there is, it's unfortunate but not a big issue if we remove this optimisation from the compiler. I'll also add a note with more design detail on rust-lang/const-eval#20 |
|
Ping from triage: |
|
☔ The latest upstream changes (presumably #66454) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@davidhewitt I suggest we run crater on this PR to see if anyone is allowing the If you are ok with this, please rebase. I'll trigger the crater run then |
davidhewitt
force-pushed
the
const_fn_memoization
branch
from
eacf695
to
06e9c54
Compare
|
2410 errors |
I'm not familiar with crater reports, but it looks like those errors already existed rather than being introduced by this PR? |
|
Indeed. |
|
Oh no, master changed again. Please rebase again |
When a const function is being evaluated, as long as all its arguments are zero-sized-types (or it has no arguments) then we can trivially memoize the evaluation result using the existing query mechanism.
davidhewitt
force-pushed
the
const_fn_memoization
branch
from
06e9c54
to
5398139
Compare
|
Rebased once again 😄 |
|
@bors r+ Thanks! |
|
📌 Commit 5398139 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 5398139 with merge 5a4558db0a7a962965661a0f260580776df72e64... |
lqd
reviewed
Nov 27, 2019
Co-Authored-By: lqd <remy.rakic+github@gmail.com>
|
I've just added a commit for a typo spotted by @lqd (many thanks). Does bors need approval again to perform the merge? |
|
yes bors will need the new commit approved @bors r=oli-obk |
|
📌 Commit a28fbd4 has been approved by |
|
and since the PR has perf implications: @bors rollup=never |
RalfJung
reviewed
Nov 27, 2019
| // | ||
| // For the moment we only do this for functions which take no arguments | ||
| // (or all arguments are ZSTs) so that we don't memoize too much. | ||
| if self.tcx.is_const_fn_raw(instance.def.def_id()) && |
There was a problem hiding this comment.
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.
I propose to instead move memoization into const_eval's find_fn implementation.
There was a problem hiding this comment.
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.
I propose to instead move memoization into const_eval's find_fn implementation.
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.
Well we might have to expand what find_fn can return, and the name might have to change reflecting that.
Is there a situation where a const fn can access global state which itself is not constant?
Right now, probably not, but as const fn get more powerful and allow unsafe 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.
Right now, probably not, but as const fn get more powerful and allow unsafe code, that seems very possible to me.
Yikes - I wasn't aware we'll one day have unsafe code in const fn!
Well we might have to expand what find_fn can return, and the name might have to change reflecting that.
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.
Right now, probably not, but as const fn get more powerful and allow unsafe code, that seems very possible to me
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.
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.
We certainly shouldn't allow that during CTFE, but that's not what I talked about. I am talking about Miri-the-standalone-tool.
There's no need for dynamic checks if we do post monomorphization checks for this situation.
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.
Right, I don't remember what I meant by that.
bors
added a commit
that referenced
this pull request
Nov 28, 2019
Add memoization for const function evaluations When a const function is being evaluated, as long as all its arguments are zero-sized-types (or it has no arguments) then we can trivially memoize the evaluation result using the existing query mechanism. With thanks to @oli-obk for mentoring me through this at RustFest Barcelona. r? @oli-obk
|
☀️ Test successful - checks-azure |
RalfJung
reviewed
Dec 15, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When a const function is being evaluated, as long as all its arguments are zero-sized-types (or it has no arguments) then we can trivially memoize the evaluation result using the existing query mechanism.
With thanks to @oli-obk for mentoring me through this at RustFest Barcelona.
r? @oli-obk