Add is_const_eval intrinsic #64683
Add is_const_eval intrinsic #64683
Conversation
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
|
rust-highfive
added
the
S-waiting-on-review
|
r? @oli-obk |
|
cc @Centril |
|
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
|
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
|
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
|
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. 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 |
|
So.. can you undo the submodule things? It's not going to break miri and we can just merge the miri PR independently The PR lgtm now, but I'll let @RalfJung and especially @Centril raise any blockers. This intrinsic could now be used in stable const fn as far as I can tell, which is quite dangerous, because it would create forward compat hazards, where we can't take the decision back without perf regressions due to having to remove SIMD stuff from const fns Also, this PR will not actually enable anyone to do SIMD stuff, because const qualif will still forbid it. Previous discussions: rust-lang/const-eval#7 From a const qualif perspective I'd prefer a "write two function bodies for the same function" approach. Essentially unsafe overloading on the constness of a function. I have no idea what the syntax would be, but it can just be compiler magic attributes if we just want it for SIMD. That said, if we want the proposed intrinsic, the way the PR looks now is how it would be |
Without upgrading miri here to a version that contains those changes, one of the tests I've added should fail :/
To clarify, it could be used within libcore/liballoc/libstd on const fns that are exposed to stable Rust, but stable Rust - don't do that! It cannot be directly used in stable Rust because it requires the
I intend to use this with
Right now we are kind of at the step "let's see if libstd can do something useful with this", and for that, an unstable intrinsic feels like the simplest solution to the problem (no need to add new syntax, etc.). If we wanted to expose this to users, then there are many ways in which we could do so syntactically, but AFAICT they are all semantically equivalent (they all allow const fns to execute different code at run-time than at compile-time). I think issue rust-lang/const-eval#7 would be good place to discuss that, but I think that at this point it would be better to try to understand what the use cases would be, and for that some experience using it would be helpful. |
|
Miri won't fail. Miri isn't run on tests In order to guarantee it not being in stable cons fns (I mean in libcore/std/SIMD ) we need to fix our intrinsic story first |
👍 -- Should block on fixing that first then? |
|
Also, as previously stated, this won't actually allow you to implement anything. What you probably want is an intrinsic that takes two function types (the zst ones) as generic arguments and calls one at compile time and the other at runtime. This way we don't have to start making the const qualif checker more complicated |
FWIW I at least do not intend to use this in stdarch, or at least, not until we have a stable feature for this, or at least an RFC. |
|
I think the worry isn't about intent, but accident |
|
Would a feature-fate just for this intrinsic do? Or what would be the appropriate way to gate this ? |
|
We need something like #61495 but additionally handling const stability |
Ah, I remember that issue, but I don't see why fixing it wouldn't suffice. We can just ban this intrinsic from being used in |
gnzlbg
force-pushed
the
is_const_eval
branch
from
d621fd1
to
4fe1b28
Compare
| use std::intrinsics::is_const_eval; | ||
|
|
||
| fn main() { | ||
| const X: bool = unsafe { is_const_eval() }; |
There was a problem hiding this comment.
So AFAICT the only remaining issue is that this currently works due to #61495 allowing all intrinsics to run in const-contexts (notice that feature(const_fn) is not enabled!).
Even if we were to fix #61495, this would still "just work" behind the const_fn feature. That would allow it to "silently" be used in APIs exposed by libstd. I'm not sure how to fix that.
There was a problem hiding this comment.
If it is fixed as I imagine it, the const fn feature gate won't have an effect on it. It would only permit a whitelist of intrinsics and forbid all others outside unleash mode
There was a problem hiding this comment.
Agreed with @oli-obk. Why would just "just fork" with const_fn? That gate won't make non-const-fn callable, and all intrinsics (except for a whitelist) should be considered non-const-fn.
There was a problem hiding this comment.
I think the "why" issue is already resolved in the updated OP description and the new test that shows an use case that works with this, but wouldn't work without it.
| let name = &*tcx.item_name(def_id).as_str(); | ||
| if name == "is_const_eval" { | ||
| return Ok(ty::Const::from_bool(tcx, true)); |
There was a problem hiding this comment.
Won't this mean that it returns true in Miri, which seems odd (for now, anyway)?
This should query a machine flag to determine the intrinsic's return value.
There was a problem hiding this comment.
Won't this mean that it returns true in Miri, which seems odd (for now, anyway)?
This returns true in miri. Were it to return false, miri would fail trying to do something it doesn't support.
There was a problem hiding this comment.
I don't think this is odd at all given that miri already provides cfg!(miri) which can be used to the same effect.
This should query a machine flag to determine the intrinsic's return value.
Does cfg!(miri) do this?
EDIT: users that want to do something else for miri can just use cfg(miri) on top of this, instead of making the result of this intrinsic be target dependent.
There was a problem hiding this comment.
The difference is that miri is more like codegen and not like const eval in its semantics. Cfgs flags are their own world independent of the runtime
There was a problem hiding this comment.
Miri is implementing Rust's run-time semantics as much as it can. So is_const_eval must return false.
If you actually want to check "is this a restricted interpreted environment that does not support SIMD", then you should pick an appropriate name for the intrinsic.
There was a problem hiding this comment.
Miri is implementing Rust's run-time semantics as much as it can. So is_const_eval must return false.
I could cfg(miri) is_const_eval to always return false, but then miri won't be able to execute any code for which is_const_eval makes sense using.
|
I share @Centril's concern. This operation has the chance to break many things about our const-story. It should really be conisdered "unsafe" with the safety contract being "the code must do the same thing no matter the return value". What worries me most, TBH, is that the PR description does not even state a motivation for this. That should definitely be added before considering to merge the PR. |
| /// | ||
| /// Code that uses this intrinsic must be extremely careful to ensure that | ||
| /// `const fn`s remain referentially-transparent independently of when they | ||
| /// are evaluated. |
There was a problem hiding this comment.
I find the reference of referential transparency here mostly unhelpful. At least, I would have no idea what exactly const code must (not) do, based on this description.
But also, this does not affect const-qualif. It doesn't actually introduce any referential intransparency. So personally I'd trim this down to "the code must do the same thing for both cases". Which leaves you wonder why this is even added.
There was a problem hiding this comment.
My understanding is that this is used in C++ to speed up the run-time code with SIMD and whatnot but that both code paths should observably behave same (for some notion of observational equivalence). https://en.cppreference.com/w/cpp/types/is_constant_evaluated
There was a problem hiding this comment.
If this SIMD code used run-time feature detection, couldn't be "just" make that report "no SIMD support" for const-eval to avoid needing any other magic intrinsic?
Either way though, this should be stated in the PR description.
There was a problem hiding this comment.
If this SIMD code used run-time feature detection, couldn't be "just" make that report "no SIMD support" for const-eval to avoid needing any other magic intrinsic?
How would that run-time feature detection thing be implemented ? 😉
There was a problem hiding this comment.
I thought that already existed for SIMD anyway?
There was a problem hiding this comment.
is_x86_feature_detected! has an if cfg!(miri) { false } branch in it, but it is not usable inside const fns. To be able to use the macro in const fns we'd need something like this, the macro uses inline assembly internally..
There was a problem hiding this comment.
How would that run-time feature detection thing be implemented ? wink
Not sure, but I don't think we need this intrinsic for it? (And if we do, the name should be is_miri.)
Is the problem just that you cannot do if in const fn? We are accumulating more and more bad hacks to work around that and I'd rather put the line down before we add intrinsics for this reason. You can for example already do
#[cfg(miri)]
const fn is_miri() { true }
#[cfg(not(miri))]
const fn is_miri() { false }There was a problem hiding this comment.
I don't think you have understood what the problem is here.
is_x86_feature_detected! needs to use inline assembly at run-time. If you want to call that from const fns, either const fns need to support inline assembly, or you need to provide a way to do something else at compile time.
If you want to "properly" emulate is_x86_feature_detected on miri then miri would need to support inline assembly. Right now, we just have a #[cfg(miri)] return false; that does nothing on miri instead. That could be improved with some "hook" that miri provides for doing run-time feature detection, that calls some function from the "miri run-time" that is implemented using inline assembly or similar. This has nothing to do with the problem being discussed here though, which is how to call is_x86_feature_detected! from a const fn, or more generally, how to be able to call efficient run-time code that uses "stuff that constant evaluation probably won't ever support" like inline assembly by providing an alternative slower implementation for constant evaluation.
There was a problem hiding this comment.
Unless I completely misunderstood const fns scope and the plan is indeed to actually support inline assembly in const fns at some point, that is.
There was a problem hiding this comment.
Unless I completely misunderstood const fns scope and the plan is indeed to actually support inline assembly in const fns at some point, that is.
😱: https://github.com/CraneStation/cranelift/issues/444#issuecomment-531574394
Yeah, we should do that IMO. But #61495 is basically about intrinsics not being actually banned right now in const fn, even when they should... Basically, the only thing that protects us from disaster (all intrinsics being const-callable) right now is that no intrinsics are actually callable on stable, except for |
Can we ban a feature gate from being used in libstd ? You propose above that the libstd run-time detection functionality should "do this somehow" instead. That would mean that normal Rust code within libstd would need to be able to detect constant evaluation somehow, and branch on that... So... that code within libstd would need to be able to use this feature, or something like this, so banning this in libstd doesn't appear to be what we want either :/ |
| x[0] == y[0] && x[1] == y[1] && x[2] == y[2] && x[3] == y[3] | ||
| } else { | ||
| unsafe { | ||
| let x = _mm_loadu_si128(&x as *const _ as *const _); |
There was a problem hiding this comment.
This is essentially what I worried about. Such code will never ever be accepted by rustc AFAIK. If you want it for experimentation, that seems fine but ultimately pointless. Writing a const qualification that could accept this would be the motherload of unsoundness dangers. My suggestion with an intrinsic, that calls one of two functions given to it depending on the context, would be much more manageable
There was a problem hiding this comment.
Long version:
In contrast to C++, we statically prove that your const fn is sane. Of course we also have escape hatches via unsafe, but that's a different story. What your example has is an if condition that works differently whether you are const evaluating or codegenning. Now, this is not a problem by itself, but the problem arises with the else arm's content. That content contains statically provable nonconst code and since it's part of a const fn that will be errored about. I'm presuming the intent is to decide whether to run const checks depending on the arm that one is in. This may seem easy in the given example, but once you involve loops, short circuiting, matching, break and continue, this can get very complex to decide. Additionally even the simple version will complicate the const qualif check a lot, making one of the most fragile things keeping Rust's safety guarantees even more fragile. If there is a less complex version (and I believe there is), we should go for that one.
There was a problem hiding this comment.
I see. Yes, the "boolean intrinsic" approach implemented here would require the constant evaluator to only try to prove things about the branches that constant evaluation actually can reach. I can see how that could get nasty.
IIUC, what you are proposing is something like:
mod intrinsics {
pub fn const_eval_select<T, U, V>(called_in_const: T, called_at_rt: U) -> V
where T: const Fn() -> V, U: Fn() -> V;
}that can be used to rewrite the example above as follows:
const fn eq_ct(x: [i32; 4], y: [i32; 4]) -> bool {
x[0] == y[0] && x[1] == y[1] && x[2] == y[2] && x[3] == y[3]
}
fn eq_rt(x: [i32; 4], y: [i32; 4]) -> bool {
let x = _mm_loadu_si128(&x as *const _ as *const _);
let y = _mm_loadu_si128(&y as *const _ as *const _);
let r = _mm_cmpeq_epi32(x, y);
let r = _mm_movemask_ps(transmute(r) );
r == 0b1111
}
const fn eq(x: [i32; 4], y: [i32; 4]) -> bool {
const_eval_select(|| eq_ct(x, y), || eq_rt(x, y))
// unclear to me how to make these closures work well here
}?
Is that correct? Maybe you could post about it in the tracking issue (rust-lang/const-eval#7) ? Your last comment there appeared to be in agreement with using a "boolean intrinsic", but it does have quite a bit of issues, so maybe we can move the discussion there.
There was a problem hiding this comment.
While closures would be neat, they seem an unnecessary complication of such a low level function
mod intrinsics {
pub fn const_eval_select<ARG, F, G, RET>(arg: ARG called_in_const: F, called_at_rt: G) -> RET;
}you could then use it as
const fn eq_ct((x, y): ([i32; 4], [i32; 4])) -> bool {
x[0] == y[0] && x[1] == y[1] && x[2] == y[2] && x[3] == y[3]
}
fn eq_rt((x, y): ([i32; 4], [i32; 4])) -> bool {
let x = _mm_loadu_si128(&x as *const _ as *const _);
let y = _mm_loadu_si128(&y as *const _ as *const _);
let r = _mm_cmpeq_epi32(x, y);
let r = _mm_movemask_ps(transmute(r) );
r == 0b1111
}
const fn eq(x: [i32; 4], y: [i32; 4]) -> bool {
const_eval_select((x, y), eq_ct, eq_rt)
// unclear to me how to make these closures work well here
}The const_eval_select function then compiles down to a call to eq_rt(arg) in llvm and is interpreted as a call to eq_ct(arg) in const eval
There was a problem hiding this comment.
This sounds great.
So how do I implement this? IIUC, I just add an intrinsic to typeck like I did here, and implement it with the "const semantics" in const-eval, and then add a different implementation for it in the llvm backend, and that's it ?
There was a problem hiding this comment.
Ideally the intrinsic checks would report errors on any illegal uses, but I don't know how easy that is to do
I can't imagine this is feasible, we'd need to analyze the "runtime code" and figure out if it does something non-const friendly, but we can't really tell much about that because the point of this feature is to be able to call SIMD intrinsics or inline assembly, which is essentially stuf we cannot reason about.
I'd be fine with saying that this intrinsic is unsafe to use, and making the user responsible for using it correctly.
There was a problem hiding this comment.
Oh that's not what I meant. I just mean that
- the signature of the two functions needs to match,
- the first function needs to be
const fn, - the return type of both functions is the same as the generic
RETparameter - the argument of both functions is the same type as the
ARGparameter
The content of the functions is indeed left to the user as per the docs you already provided
There was a problem hiding this comment.
Ah, gotcha. Hm, what about just saying:
mod intrinsics {
pub fn const_eval_select<ARG, F, G, RET>(
arg: ARG called_in_const: F, called_at_rt: G
) -> RET
where F: FnOnce(ARG) -> RET, G: FnOnce(ARG) -> RET
}?
That way, typeck enforces that the arguments and return types match, and well, that they are function types. I don't know how to check that the first function is const in typeck though, but maybe it is possible to do during const qualif.
There was a problem hiding this comment.
I thought we wouldn't go for closures, to keep things simple? Then it would just be
pub fn const_eval_select<ARG, RET>(
arg: ARG,
called_in_const: fn(ARG) -> RET,
called_at_rt: fn(ARG) -> RET,
) -> RETThere was a problem hiding this comment.
function pointers are not the same thing as the zst arguments and won't do us any favours here. The intrinsic would not be able to figure out which function to call and would have to insert function pointer calls and we can't have const function pointers. While all solvable problems, I'd rather not go down that road
I mean you can add Fn bounds as you suggestd, that should suffice indeed.
that they are function types
That's not checked, it could be a closure. Thus it needs an extra check to make sure only functions are used as generic arguments
|
☔ The latest upstream changes (presumably #64766) made this pull request unmergeable. Please resolve the merge conflicts. |
|
(Moving to top-level as this is becoming a high-level discussion, not a comment related to some particular part of this PR.)
That might be related to the fact that you never properly explained what the problem is. ;) As @oli-obk explained, for the problem you seem to want to solve ("const-qualif rejects my inline assembly" is my current guess, please correct if I am wrong), the intrinsic is of no use. So when you talk about "allowing users to write different code for the different execution contexts of const fns" I naturally assumed you are talking about the dynamic part only, ignoring the checks, but at that point I think we can have better solutions. Now it seems like you do care about the checks, so the PR description is misleading (and maybe I was just assuming too much about how familiar everyone is with the architecture of this). Also notice that for the Miri interpreter, inline assembly in dead code is no problem. And yet we talked about Miri here. I feel like there are at least 2, maybe more, issues being conflated in this PR. So maybe start with a clear problem statement and let's go from there? Here's what I think I gathered from the conversation so far:
@oli-obk's intrinsic with two closures (basically a user-defined if have_simd() { do_simd() } else { do_fallback() }And this would basically have to turn into if_miri(
|| do_fallback(),
|| if have_simd() { do_simd() } else { do_fallback() } // not const-checked
)So, we have code duplication and the fact that A different solution might be to make Isn't there also fn foo() {
if cfg_feature_enabled!("avx") {
unsafe { foo_avx() } // not const-checked
} else if cfg_feature_enabled!("sse4") {
unsafe { foo_sse4() } // not const-checked
} else {
foo_impl() // use the default version
}
}can work. |
|
The problem is explained in rust-lang/const-eval#7, and this PR implements the solution proposed by @oli-obk in their comment there: rust-lang/const-eval#7 (comment) The other issues that @oli-obk raised here are new, and were not raised there.
Without a way to detect
I would prefer the C++ solution, where Rust performs the |
That is |
EDIT (see next comment): |
|
@bjorn3 you are right, if a feature is enabled at compile-time, Code that wants to run under |
|
The easiest way out it is to have miri disable all the features. (as in -Zunleash-miri-... -> all features disappears) |
|
Ping from triage. @oli-obk any updates on this? Thanks. |
oli-obk
added
S-waiting-on-author
JohnCSimon
added
the
S-inactive
|
Ping from triage. Thank you! |
Right now, users often need to pick between writing code in a
const fnfriendly way that can be executed at compile-time, or run-time performance (e.g. code that usescore::archintrinsics, inline assembly, etc.). See rust-lang/const-eval#7.The unsafe
core::intrinsic::is_const_evalreturnstrueif it is evaluated at compile-time andfalseotherwise, allowing users to write different code for the different execution contexts ofconst fns.There is a PR for this in miri upstream: rust-lang/miri#959