Repository navigation
Conversation
|
Some changes occurred in compiler/rustc_attr_ir cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_attr_parsing cc @jdonszelmann, @JonathanBrouwer Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
|
Error: Failed to set assignee to
Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip. |
|
r? RalfJung |
|
|
| @@ -0,0 +1,28 @@ | |||
| //@ run-pass | |||
| //@ ignore-backends: gcc cranelift | |||
There was a problem hiding this comment.
I think it should still work for these backends, they should just get the fallback body
There was a problem hiding this comment.
I agree that it will work, but this test does Crimes (tm) to detect whether the fallback body or the actual intrinsics was used, and I don't think that will work on a backend that always uses the fallback body :-)
(if you object strongly enough to the crimes I can think of other robust ways to test this ;-))
There was a problem hiding this comment.
Yup that's fair, I can't think of a better way without having a cfg to detect the backend
|
Cc @folkertdev |
39047e6 to
a0ad71b
Compare
|
Can you check that all places in the compiler that check for |
| codegen_attrs.llvm_intrinsic | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
Please also update cg_clif.
There was a problem hiding this comment.
How should cg_clif be updated? We clearly don't want to just call codegen_llvm_intrinsic_call as-is today (the entire point of this attribute is to have LLVM intrinsics with fallback bodies for cranelifts and the like).
IMO, it is reasonable for now to leave cg_clif as-is, and in a follow-up (or when cg_clif wishes to implement more architecture-specific LLVM intrinsics), the cg_clif can be updated accordingly; alternatively, I can split off codegen_llvm_intrinsic_call in a function returning a Result<>, and then call the fallible version for rustc_llvm_intrinsic functions and a version that emits an error for Instance::LlvmIntrinsic. WDYT?
There was a problem hiding this comment.
How should cg_clif be updated?
Apply the above diff for getting the intrinsic name to cg_clif and add support for calling the fallback. If I understand correctly #[rustc_llvm_intrinsic] will result in InstanceKind::LlvmIntrinsic, which cg_clif will unconditionally push through the llvm intrinsic emulation code without any chance for the fallback code to run currently. For #[rustc_intrinsic] there is already fallback code. You can mirror that.
There was a problem hiding this comment.
#[rustc_llvm_intrinsic] should not result in a InstanceKind::LlvmIntrinsic with the current design, it should just be a normal InstanceKind::Item. So clif should work as-is. In particular, manually running the newly added test with the cranelift backend gives the following:
luca@xps ~/rust rustc_llvm_intrinsic $ ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc --cfg fallback -Zcodegen-backend=cranelift tests/ui/intrinsics/rustc-llvm-intrinsic.rs && ./rustc-llvm-intrinsic
warning: the feature `rustc_attrs` is internal to the compiler or standard library
--> tests/ui/intrinsics/rustc-llvm-intrinsic.rs:6:12
|
6 | #![feature(rustc_attrs)]
| ^^^^^^^^^^^
|
= note: using it is strongly discouraged
= note: `#[warn(internal_features)]` on by default
warning: 1 warning emitted
luca@xps ~/rust rustc_llvm_intrinsic $ ./build/x86_64-unknown-linux-gnu/stage1/bin/rustc --cfg default -Zcodegen-backend=cranelift tests/ui/intrinsics/rustc-llvm-intrinsic.rs && ./rustc-llvm-intrinsic
warning: the feature `rustc_attrs` is internal to the compiler or standard library
--> tests/ui/intrinsics/rustc-llvm-intrinsic.rs:6:12
|
6 | #![feature(rustc_attrs)]
| ^^^^^^^^^^^
|
= note: using it is strongly discouraged
= note: `#[warn(internal_features)]` on by default
warning: 1 warning emitted
thread 'main' (1742231) panicked at tests/ui/intrinsics/rustc-llvm-intrinsic.rs:24:9:
assertion `left == right` failed
left: 6
right: 8
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
which matches what I think should happen.
There was a problem hiding this comment.
#[rustc_llvm_intrinsic] should not result in a InstanceKind::LlvmIntrinsic with the current design, it should just be a normal InstanceKind::Item.
That is inconsistent with #[rustc_intrinsic]. Is there any reason it was done this way?
There was a problem hiding this comment.
True. I guess that "mangling the name in IntrinsicDef::name" might not be so messy after all.
Using the llvm_intrinsic name directly would preclude having rustc_intrinsic(llvm_intrinsic = ..., gcc_intrinsic = ...). But this is also something we can fairly easily change again later so I'd be fine either way.
So... does that sound like a reasonable plan to you @bjorn3? Should we file an MCP? The impact is fairly limited to stdarch & codegen backends, so not sure if that is needed.
There was a problem hiding this comment.
Sounds reasonable. No idea if we need an MCP.
There was a problem hiding this comment.
@davidtwco @BoxyUwU any thoughts on whether refactoring our handling of LLVM intrinsics like this needs an MCP?
Also Cc @Amanieu @folkertdev @sayantn as stdarch is the main user of these intrinsics.
There was a problem hiding this comment.
Personally I think it's fine without it, it's gotten a reasonable amount of discussion around #t-libs/stdarch > Bad codegen for a bunch of intrinsics already and haven't been many objections.
Mild preference for #[rustc_intrinsic(llvm = ..., gcc = ...)] to keep things a slight bit more terse than repeating intrinsic multiple times.
There was a problem hiding this comment.
Codegen backends don't look at the item name of intrinsics. They look at the output of
tcx.intrinsic(instance), which we could derive from the#[rustc_intrinsic(llvm_intrinsic = "...")]argument.
What I am imagining is that in IntrinsicDef, the name field changes to an Option or so. That way these LLVM intrinsics can avoid having a name that could be confused with a Rust intrinsic.
It could also be an enum like
enum IntrinsicName {
RustIntrinsic(Symbol),
BackendIntrinsic {
llvm: Option<Symbol>,
gcc: Option<Symbol>,
}
}Or, in the first version without support for GCC intrinsics:
enum IntrinsicName {
RustIntrinsic(Symbol),
BackendIntrinsic {
llvm: Symbol,
}
}
I gave a look, and I think so, but I am definitely not knowledgeable enough of compiler internals to be sure. |
ebce41f to
90416b5
Compare
|
Some changes occurred to constck cc @fee1-dead Some changes occurred to the CTFE machinery
cc @bjorn3 |
This comment has been minimized.
This comment has been minimized.
90416b5 to
c906c98
Compare
|
cc @rust-lang/clippy |
| // Having a fallback body that does not match the | ||
| // intrinsic is not a good idea, but we do that here for testing. | ||
| #[rustc_intrinsic(llvm = "llvm.fshl.i64")] | ||
| const fn sometimes_fshl(a: i64, b: i64, sh: i64) -> i64 { | ||
| a + b + sh | ||
| } |
There was a problem hiding this comment.
Can this be used on functions without a fallback body, like we have for a lot of intrinsics in library/core/src/intrinsics/mod.rs? Such as
#[rustc_intrinsic(llvm = "llvm.fshl.i64")]
const fn sometimes_fshl(a: i64, b: i64, sh: i64) -> i64;So we could start migrating from #[link_name = "llvm..."] toward this new syntax even if we don't have fallback bodies. Which would also give us the option of adding GCC or clif intrinsics.
There was a problem hiding this comment.
Actually that syntax is a little weird so if it doesn't work easily, probably fine to do in another PR
There was a problem hiding this comment.
I added a test that seems to pass, so yes:
//@ run-pass
//@ needs-backends: llvm
#![feature(intrinsics)]
#[rustc_intrinsic(llvm = "llvm.fshl.i64")]
fn fshl(a: i64, b: i64, sh: i64) -> i64;
fn main() {
let res = fshl(1, 2, 3);
assert_eq!(res, 8);
} c906c98 to
b202b07
Compare
|
☔ The latest upstream changes (presumably #165234) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
View all comments
This is meant for usage in core::arch, to define the platform-specific LLVM intrinsics that correspond to each intrinsic.
LLM usage: I used an LLM to point me at places in the code from which to start the implementation, but I did not generate any code. The main non-trivial input from the LLM was to prevent inlining of callees annotated with rustc_llvm_intrinsic.
r? RalfJ