-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
rustc_mir: split qualify_consts' "value qualification" bitflags into separate computations. #58403
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. |
Abi::RustIntrinsic | | ||
Abi::PlatformIntrinsic => { | ||
assert!(!cx.tcx.is_const_fn(def_id)); | ||
match &cx.tcx.item_name(def_id).as_str()[..] { |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 fn
s
- only "rvalue-to-
- "explicit promotion" / "forced promotion"
- e.g. "
const
args" (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
const
item, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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