Organize intrinsics const evaluability checks #61835
Closed
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
|
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @oli-obk |
oli-obk
reviewed
Jun 14, 2019
vertexclique
force-pushed
the
organize-intrinsics-promotion-checks
branch
from
60ae651
to
0c2a54d
Compare
oli-obk
reviewed
Jun 14, 2019
oli-obk
reviewed
Jun 14, 2019
oli-obk
reviewed
Jun 14, 2019
oli-obk
reviewed
Jun 14, 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 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 |
oli-obk
reviewed
Jun 14, 2019
vertexclique
changed the title
|
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 #61817) made this pull request unmergeable. Please resolve the merge conflicts. |
RalfJung
reviewed
Jun 16, 2019
eddyb
reviewed
Jun 16, 2019
src/librustc/ty/constness.rs
Outdated
| @@ -68,14 +69,54 @@ impl<'tcx> TyCtxt<'tcx, 'tcx> { | |||
|
|
|||
|
|
|||
| pub fn provide<'tcx>(providers: &mut Providers<'tcx>) { | |||
| /// only checks whether the function has a `const` modifier | |||
| fn is_const_evaluatable(tcx: TyCtxt<'tcx, 'tcx>, def_id: DefId) -> bool { | |||
There was a problem hiding this comment.
I would not have this as a separate function, but rather use it before the usage of FnLikeNode, so intrinsics' definitions aren't checked for const fn (as they syntactically cannot be const fn).
There was a problem hiding this comment.
(as they syntactically cannot be const fn).
Shouldn't we just allow them to be that syntactically?
There was a problem hiding this comment.
Didn't you say you tried that once and it did not work?
Also, that would allow nightly users to declare intrinsics as const. There might be ways to cause unsoundness there. Wouldn't that be a problem?
There was a problem hiding this comment.
Didn't you say you tried that once and it did not work?
It would be a lot of work
Also, that would allow nightly users to declare intrinsics as const. There might be ways to cause unsoundness there. Wouldn't that be a problem?
Well... if there's an implementation in the miri-engine, then they can do whatever unstable fun they want I guess?
There was a problem hiding this comment.
Yes. And that might subvert properties that const fn otherwise have.
There was a problem hiding this comment.
I only consider this a problem in libstd. If you are using nightly features you can already brick properties that we aren't sure about yet, so...
There was a problem hiding this comment.
One of these days we should change intrinsics so they don't abuse extern "..." {...} anymore and then we can have const fn transmute or w/e. As for not allowing const fn on intrinsics that shouldn't be, I suppose that should go in rustc_typeck's list of intrinsic signatures.
There was a problem hiding this comment.
We could just move to making them lang items and having libcore implement them by recursing on themselves and then have the compiler overwrite them. Then they'd be regular functions (and we used this trick on one intrinsic already:
Line 197 in 605ea9d
There was a problem hiding this comment.
That would make it harder to figure out that a function is an intrinsic. Also real_drop_in_place actually codegens to real functions, while intrinsics always inline their code at the caller site. Another difference between intrinsics and (lang item) functions is that intrinsics cant be turned into function pointers, while (lang item) functions can be.
There was a problem hiding this comment.
@oli-obk sigh we don't have to re-use lang items, we can have a different mechanism (but also attribute-based) for them. I've been meaning to do this for ages but @nikomatsakis wanted an RFC which I never got around to writing, oops.
oli-obk
reviewed
Jun 17, 2019
oli-obk
added
S-waiting-on-author
|
Ping from triage @vertexclique are there any further updates on this? |
|
I am going to update this when I have time on the weekend. |
oli-obk
reviewed
Sep 28, 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 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 |
oli-obk
reviewed
Sep 28, 2019
| @@ -1322,23 +1283,10 @@ impl<'a, 'tcx> Visitor<'tcx> for Checker<'a, 'tcx> { | |||
| match self.tcx.fn_sig(def_id).abi() { | |||
| Abi::RustIntrinsic | | |||
| Abi::PlatformIntrinsic => { | |||
| assert!(!self.tcx.is_const_fn(def_id)); | |||
| assert!(self.tcx.is_const_fn(def_id), | |||
There was a problem hiding this comment.
Oh... this is not for const fn, but for normal functions. So you can remove the assert since it's now useless and you can also remove the RustIntrinsic pattern above, as we only seem to care about SIMD_shuffle which is a PlatformIntrinsic
oli-obk
reviewed
Sep 28, 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 |
Moves fn checking in const context into the Checker. This is better for reusing via Checker api.
|
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 #64470) made this pull request unmergeable. Please resolve the merge conflicts. |
oli-obk
reviewed
Sep 29, 2019
| | "unchecked_shr" | ||
| | "rotate_left" | ||
| | "rotate_right" | ||
| | "add_with_overflow" |
There was a problem hiding this comment.
The wrapping intrinsics seem to be missing from this list. The errors mean the check is working! Awesome! Please add anything to this list that is needed to get the tests passing
|
Hi @vertexclique! After #64470, which is necessary to enable |
|
Ping from triage. Thank you! |
|
Pinging again from triage. Thank you! |
|
I need to debug this even after I resolve the conflicts there are interesting blacklisting occurring for some intrinsics. @oli-obk knows about this situation. Weekend awaits! |
|
Ping from Triage: Hi @vertexclique - any updates? |
|
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 #65188) made this pull request unmergeable. Please resolve the merge conflicts. |
RalfJung
reviewed
Oct 31, 2019
Comment on lines
+532
to
+535
| let is_const_fn = | ||
| cx.tcx.is_const_fn(def_id) || | ||
| cx.tcx.is_unstable_const_fn(def_id).is_some() || | ||
| cx.is_const_panic_fn(def_id); |
There was a problem hiding this comment.
Isn't this IsNotPromotable? Shouldn't that check the rustc_promotable attribute instead of allowing all const fn?
There was a problem hiding this comment.
Ah dang I might have mixed this up again with IsNotImplicitlyPromotable...
There was a problem hiding this comment.
Note that this can't go wrong anymore, without triggering a mismatch with promote_consts' copy of the logic (although we should make sure this PR updates both properly), which would ICE.
@ecstatic-morse and I will be removing the copy from qualify_consts in a week or two (maybe we just need to wait for beta to branch?), so IsNot(Implicitly) Promotable is going away soon!
There was a problem hiding this comment.
I just hope we get the intrinsic thing landed as part of this refactor, it's been 4 months or so.^^
There was a problem hiding this comment.
Oh yeah this PR can land at any time, and I think promote_consts even has the right code already, without another copy of the list of intrinsics.
There was a problem hiding this comment.
Okay, but what about the new const checking pass? Does that still allow calling intrinsics?
There was a problem hiding this comment.
That's a good question for @ecstatic-morse.
There was a problem hiding this comment.
I reserved a spot for these checks here. I think we just need to call the is_const_intrinsic from that spot? I dunno if this PR makes additional changes to the validation logic though.
There was a problem hiding this comment.
I missed that this was baked into in_const_fn now (see below), I think this PR just needs to remove that return along with the FIXME above it.
|
@vertexclique so what is the status? This has been dragging on for quite a while at this point. Mind if I take over? I'd like to finally settle this. |
| match tcx.fn_sig(def_id).abi() { | ||
| Abi::RustIntrinsic | | ||
| Abi::PlatformIntrinsic => { | ||
| match &tcx.item_name(def_id).as_str()[..] { |
There was a problem hiding this comment.
Could we change this to match on a Symbol instead? This version is needlessly slow.
There was a problem hiding this comment.
Also, @_nnethercote has been trying to remove queries that are easy to compute to improve performance. I think this could just be a function.
Oh, this isn't actually exported in providers. My bad.
|
Ping from triage cc: @oli-obk @ecstatic-morse @RalfJung you may as well take over... |
|
Closing in favour of #66275 |
bors
added a commit
that referenced
this pull request
Dec 4, 2019
…r=RalfJung Organize intrinsics promotion checks cc @vertexclique supersedes #61835 r? @RalfJung
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.
Organize intrinsics const evaluability checks.