-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Don't report any errors in lower_intrinsics
.
#115602
Conversation
…typecked before.
(rustbot has picked a reviewer for you, use r? to override) |
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
if self.tcx().is_intrinsic(def_id) { | ||
match self.tcx().item_name(def_id) { | ||
sym::simd_shuffle => { | ||
if !matches!(args[2], Operand::Constant(_)) { |
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 believe there are a couple of cases where stdarch has a shuffle where the last arg is not an Operand::Constant
, but is still constant after LLVM level constprop. I do want this to be fixed though. Handling this in cg_clif requires a bunch of non-trivial code: https://github.com/bjorn3/rustc_codegen_cranelift/blob/0559de65672243a97d6cd6e6ee6a8e8c291ef4ce/src/constant.rs#L476C1-L575 (called from https://github.com/bjorn3/rustc_codegen_cranelift/blob/0559de65672243a97d6cd6e6ee6a8e8c291ef4ce/src/intrinsics/simd.rs#L171-L172) AFAIK stdarch's test suite doesn't get run in rust's CI. Only in the CI of rust-lang/stdarch.
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 just moved the check. We were already checking this. I think the const propagated ones haven't been working for a long time
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.
Looks like it is indeed unreachable for simd_shuffle
now, but for simd_insert
and simd_extract
it is still possible and necessary for stdarch.
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.
every simd_insert
usage outside of ui/codegen tests seems to be using constants
for simd_extract
, everything but some extract helpers for #60637 are using constants
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.
and even those helpers are only used in tests
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.
In stdarch there are a couple of IMM8 as u32
which is not a constant. It has to be const { IMM8 as u32 }
.
@bors r+ |
…henkov Don't report any errors in `lower_intrinsics`. Intrinsics should have been type checked earlier. This is part of moving all mir-opt diagnostics early enough so that they are reliably emitted even in check builds: rust-lang#49292 (comment)
☀️ Test successful - checks-actions |
Finished benchmarking commit (c5775a7): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 628.665s -> 629.749s (0.17%) |
Intrinsics should have been type checked earlier.
This is part of moving all mir-opt diagnostics early enough so that they are reliably emitted even in check builds: #49292 (comment)