rustc_mir: split qualify_consts' "value qualification" bitflags into separate computations.#58403
rustc_mir: split qualify_consts' "value qualification" bitflags into separate computations.#58403bors merged 10 commits intorust-lang:masterfrom
Conversation
|
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
|
@eddyb would you mind running |
|
It accepts strictly more and if our CI doesn't run those tests that sounds like an infra bug. |
There was a problem hiding this comment.
This pattern seems to mostly (not entirely) duplicate the whitelist check in the qualify_min_const.. logic; de-duplicate it by using that function?
|
r? @oli-obk |
|
This comment has been minimized.
This comment has been minimized.
a8ec409 to
0924a19
Compare
|
@bors r=oli-obk |
|
📌 Commit 0924a19d19450b882dc716b7f57ae9f2b290baf8 has been approved by |
|
In-between-rollups-filler, @bors p=1 |
|
⌛ Testing commit 0924a19d19450b882dc716b7f57ae9f2b290baf8 with merge cbc038fcc0bc89c385b42a3013b319d9697f0700... |
|
💔 Test failed - checks-travis |
|
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 |
|
Help save the climate by running |
|
Ugh, it would be tricky to keep the old message order. |
|
I think nll has separate files if the output differs. Changing the order is fine. |
|
⌛ Testing commit f04424a with merge 8db20450bdff0e7852dd73ad2d92be6aa6f8f9a4... |
|
💔 Test failed - checks-travis |
|
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 |
|
@bors retry 3 hour timeout |
rustc_mir: split qualify_consts' "value qualification" bitflags into separate computations. Prerequisite for computing those bits through a dataflow algorithm ~~(which I might do in this PR later)~~. This PR should not change behavior overall, other than treating `simd_shuffle*` identically to `#[rustc_args_required_const]` (maybe we should just have `#[rustc_args_required_const]` on the intrinsic imports of `simd_shuffle*`? cc @gnzlbg) cc @oli-obk @alexreg
|
☀️ Test successful - checks-travis, status-appveyor |
| let allowed = cx.mode == Mode::Static || cx.mode == Mode::StaticMut; | ||
|
|
||
| !allowed || | ||
| cx.tcx.get_attrs(static_.def_id).iter().any(|attr| attr.check_name("thread_local")) |
There was a problem hiding this comment.
My head hurts when reading this, everything is negated.^^
| } | ||
|
|
||
| // Refers to temporaries which cannot be promoted as | ||
| // promote_consts decided they weren't simple enough. |
There was a problem hiding this comment.
That's weird, how is this meaningfully different from the "promotion-only" checks that are done in the NotConst thing above?
| _args: &[Operand<'tcx>], | ||
| _return_ty: Ty<'tcx>, | ||
| ) -> bool { | ||
| if cx.mode == Mode::Fn { |
There was a problem hiding this comment.
This conditional is confusing, wouldn't it make more sense to simply only run this analysis when we care about promotion / only use the result of this analysis when we care about promotion?
At the least, things would be slightly more clear if all methods in this impl early-aborted for the "out of scope" case (cx.mode != Mode::Fn).
There was a problem hiding this comment.
I was discussing IsNotPromotable with @oli-obk... it's not exactly clear what to make of it, but the names are definitely wrong.
Promotability is tricky because it differs between compile-time and run-time.
Ideally we would just get rid of IsNotPromotable, but at least one run-time thing (#[rustc_const_arg]) is stable and differs in desired behavior from "promotion involving const fn calls".
There was a problem hiding this comment.
it differs between compile-time and run-time.
I am not sure what you mean by this. Do you mean the analysis is different in const context?
There was a problem hiding this comment.
I mean we allow promoting &const_fn() in a const, given that interior mutability and drop conditions are met, while at runtime const_fn also needs #[rustc_promotable].
There was a problem hiding this comment.
Okay so by "at runtime" you mean "in run-time code".
However, currently there are also a bunch of checks in NotConst that have to do with promotability, from what I can see.
There was a problem hiding this comment.
Like I said, some of the naming is wrong/misleading and should be changed.
IIRC, IsNotConst should have the IsNotPromotable name, and our current IsNotPromotable should be ...checks IRC log... IsNotImplicitlyPromotable / MayCallNotImplicitlyPromotableConstFn / MayCallUnpredictableConstFn.
(I'm not sure what @oli-obk actually prefers)
There was a problem hiding this comment.
To expand a bit: the main point was that we should distinguish between:
- "implicit promotion"
- only "rvalue-to-
'static" AFAIK - requires
#[rustc_promotable]on calledconst fns
- only "rvalue-to-
- "explicit promotion" / "forced promotion"
- e.g. "
constargs" (whether#[rustc_const_arg]or a different system) - it can't compile without being promoted, so we can allow pretty much everything we do in a
constitem, modulo the "SSA tree" shape required to actually perform the promotion on MIR (we'd need something like VSDG to allow more)
- e.g. "
We should figure out how to avoid this discussion getting lost though, heh.
There was a problem hiding this comment.
Maybe HasNoConstFnCalls? (I know it's wrong in the presence of rustc_promotable)
| interior mutability, create a static instead"); | ||
| } | ||
| } | ||
| } else { |
There was a problem hiding this comment.
This is an else clause after a negated conditional: if x != y { ... } else { ... }. Would be clearer to swap the branches.
There was a problem hiding this comment.
Nope! this is the else from if qualifs[HasMutInterior] {.
Some early returns might help, though.
Retire `IsNotConst` naming This naming scheme caused a lot of confusion lately (including ICEs) due to misrefactored code. Also clean up the initialization code for said flag. r? @eddyb previous discussions: rust-lang#58784 (comment) rust-lang#58403 (comment)
Retire `IsNotConst` naming This naming scheme caused a lot of confusion lately (including ICEs) due to misrefactored code. Also clean up the initialization code for said flag. r? @eddyb previous discussions: rust-lang#58784 (comment) rust-lang#58403 (comment)
Retire `IsNotConst` naming This naming scheme caused a lot of confusion lately (including ICEs) due to misrefactored code. Also clean up the initialization code for said flag. r? @eddyb previous discussions: rust-lang#58784 (comment) rust-lang#58403 (comment)
Retire `IsNotConst` naming This naming scheme caused a lot of confusion lately (including ICEs) due to misrefactored code. Also clean up the initialization code for said flag. r? @eddyb previous discussions: rust-lang#58784 (comment) rust-lang#58403 (comment)
Prerequisite for computing those bits through a dataflow algorithm
(which I might do in this PR later).This PR should not change behavior overall, other than treating
simd_shuffle*identically to#[rustc_args_required_const](maybe we should just have#[rustc_args_required_const]on the intrinsic imports ofsimd_shuffle*? cc @gnzlbg)cc @oli-obk @alexreg