-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Implement black_box
using intrinsic
#87916
Conversation
Some changes occured to rustc_codegen_cranelift cc @bjorn3 Some changes occured to the CTFE / Miri engine cc @rust-lang/miri |
Looks pretty nice. And in the future we have freedom to implement this in a better or otherwise backend specific manner better. |
@bors delegate=nbdd0121 (feel free to r=nagisa once the |
✌️ @nbdd0121 can now approve this pull request |
@bors r=nagisa |
📌 Commit 3cf2a69 has been approved by |
dummy | ||
#[cfg(not(bootstrap))] | ||
{ | ||
crate::intrinsics::black_box(dummy) |
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.
Longer-term we might even want to directly reexport the intrinsic instead of using a wrapper function... but that's blocked on the fn ptr reification PR.
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 guess that's also blocked by path component stability
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.
Well, we already reexport transmute
and probably will reexport copy
/copy_nonoverlapping
soon (we did but it was reverted due to the fn ptr reification problem).
So, while the lack of path component stability is a problem, it's not necessarily a blocker.
Anyway, even the exported function is currently still unstable; this is a discussion to be had at stabilization time.
@bors r=nagisa |
📌 Commit f98d540 has been approved by |
Implement `black_box` using intrinsic Introduce `black_box` intrinsic, as suggested in rust-lang#87590 (comment). This is still codegenned as empty inline assembly for LLVM. For MIR interpretation and cranelift it's treated as identity. cc `@Amanieu` as this is related to inline assembly cc `@bjorn3` for rustc_codegen_cranelift changes cc `@RalfJung` as this affects MIRI r? `@nagisa` I suppose
Possibly failed in rollup: #87939 (comment) |
It seems to me that the codegen is better with the intrinsic approach and a memcpy is eliminated from Previously it's like (expressed in MIR): fn black_box(_1: T) -> T {
let mut _0: T; // return place
let mut _2: &mut T;
bb0: {
_2 = &mut _1;
llvm_asm!(""::"r"(_2):"memory":"volatile");
_0 = move _1; // LLVM can't optimize out
return;
}
} The intrinsic approach it's codegenned more like: fn black_box(_1: T) -> T {
let mut _0: T; // return place
let mut _2: &mut T;
bb0: {
_0 = move _1; // LLVM can optimize out
_2 = &mut _0;
llvm_asm!(""::"r"(_2):"memory":"volatile");
return;
}
} I think it'd be sufficient just to change the pattern so that "main" is also matched? |
@nagisa A sanitize test needs to be fixed (last commit). PTAL |
Please squash the commits. r=me once its done. |
The new implementation allows some `memcpy`s to be optimized away, so the uninit value in ui/sanitize/memory.rs is constructed directly onto the return place. Therefore the sanitizer now says that the value is allocated by `main` rather than `random`.
@bors r=nagisa |
📌 Commit 1fb1643 has been approved by |
Implement `black_box` using intrinsic Introduce `black_box` intrinsic, as suggested in rust-lang#87590 (comment). This is still codegenned as empty inline assembly for LLVM. For MIR interpretation and cranelift it's treated as identity. cc `@Amanieu` as this is related to inline assembly cc `@bjorn3` for rustc_codegen_cranelift changes cc `@RalfJung` as this affects MIRI r? `@nagisa` I suppose
☀️ Test successful - checks-actions |
…arth Rollup of 4 pull requests Successful merges: - rust-lang#87916 (Implement `black_box` using intrinsic) - rust-lang#87922 (Add c_enum_min_bits target spec field, use for arm-none and thumb-none targets) - rust-lang#87953 (Improve formatting of closure capture migration suggestion for multi-line closures.) - rust-lang#87965 (Silence non_fmt_panic from external macros.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Introduce
black_box
intrinsic, as suggested in #87590 (comment).This is still codegenned as empty inline assembly for LLVM. For MIR interpretation and cranelift it's treated as identity.
cc @Amanieu as this is related to inline assembly
cc @bjorn3 for rustc_codegen_cranelift changes
cc @RalfJung as this affects MIRI
r? @nagisa I suppose