Skip to content

Conversation

so-lovely
Copy link

@so-lovely so-lovely commented Aug 31, 2025

Fixes: #145248

@rustbot
Copy link
Collaborator

rustbot commented Aug 31, 2025

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 31, 2025
@rust-log-analyzer
Copy link
Collaborator

The job tidy failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
##[endgroup]
[TIMING:end] tool::ToolBuild { build_compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu, forced_compiler: false }, target: x86_64-unknown-linux-gnu, tool: "tidy", path: "src/tools/tidy", mode: ToolBootstrap, source_type: InTree, extra_features: [], allow_features: "", cargo_args: [], artifact_kind: Binary } -- 9.892
[TIMING:end] tool::Tidy { compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu, forced_compiler: false }, target: x86_64-unknown-linux-gnu } -- 0.000
fmt check
Diff in /checkout/compiler/rustc_ty_utils/src/layout.rs:569:
             // SIMD-like struct detection and optimization with conservative safety constraints
             // Check if this is a user-defined struct like Vector<T, const N: usize>([T; N])
             // that should be treated as SIMD-friendly even without #[repr(simd)]
-            if def.is_struct() 
-                && def.variants().len() == 1 
+            if def.is_struct()
+                && def.variants().len() == 1
                 && def.variant(FIRST_VARIANT).fields.len() == 1
-                && def.did().is_local()   // Only user crate types
+                && def.did().is_local()
+            // Only user crate types
             {
                 // Safety: exclude system crates by checking crate name
                 let crate_name = tcx.crate_name(def.did().krate);
Diff in /checkout/compiler/rustc_ty_utils/src/layout.rs:579:
                 let crate_name_str = crate_name.as_str();
-                
+
                 // Exclude known system/compiler crates
-                if !matches!(crate_name_str, 
-                    "core" | "std" | "alloc" | "proc_macro" | "test" | 
-                    "rustc_std_workspace_core" | "rustc_std_workspace_alloc" |
-                    "compiler_builtins" | "libc" | "unwind" | "panic_abort" | 
-                    "panic_unwind" | "adler2" | "object" | "memchr"
+                if !matches!(
+                    crate_name_str,
+                    "core"
+                        | "std"
+                        | "alloc"
+                        | "proc_macro"
+                        | "test"
+                        | "rustc_std_workspace_core"
+                        | "rustc_std_workspace_alloc"
+                        | "compiler_builtins"
+                        | "libc"
+                        | "unwind"
+                        | "panic_abort"
+                        | "panic_unwind"
+                        | "adler2"
+                        | "object"
+                        | "memchr"
                 ) {
                     let field_ty = def.variant(FIRST_VARIANT).fields[FieldIdx::ZERO].ty(tcx, args);
-                    
+
                     // Check if the single field is an array of SIMD-friendly types
                     if let ty::Array(element_ty, array_len) = field_ty.kind() {
                         if is_simd_friendly_element_type(*element_ty) {
Diff in /checkout/compiler/rustc_ty_utils/src/layout.rs:593:
-                            if let Some(len) = extract_const_value(cx, ty, *array_len)?
-                                .try_to_target_usize(tcx) 
+                            if let Some(len) =
+                                extract_const_value(cx, ty, *array_len)?.try_to_target_usize(tcx)
                             {
                                 // Common SIMD sizes: 4, 8, 16 (conservative range)
                                 if matches!(len, 4 | 8 | 16) {
Diff in /checkout/compiler/rustc_ty_utils/src/layout.rs:598:
                                     // REQUIRE explicit alignment hint - this is the key safety measure
-                                    let has_simd_alignment = def.repr().align
-                                        .map_or(false, |align| {
+                                    let has_simd_alignment =
+                                        def.repr().align.map_or(false, |align| {
                                             let align_bytes = align.bytes();
                                             // Must have 16+ byte alignment to suggest SIMD intent
                                             align_bytes >= 16 && align_bytes.is_power_of_two()
Diff in /checkout/compiler/rustc_ty_utils/src/layout.rs:604:
                                         });
-                                    
+
                                     // Only apply if user explicitly requested SIMD-like alignment
                                     if has_simd_alignment {
                                         let e_ly = cx.layout_of(*element_ty)?;
Diff in /checkout/compiler/rustc_ty_utils/src/layout.rs:609:
-                                        return Ok(map_layout(cx.calc.simd_type(e_ly, len, false))?);
+                                        return Ok(map_layout(
+                                            cx.calc.simd_type(e_ly, len, false),
+                                        )?);
                                     }
                                 }
                             }
Diff in /checkout/compiler/rustc_ty_utils/src/layout.rs:614:
                     }
---
         // Floating-point types - most common SIMD use case
         ty::Float(_) => true,
-        // Integer types - common for SIMD  
+        // Integer types - common for SIMD
         ty::Int(_) | ty::Uint(_) => true,
         // Bool can be vectorized in some contexts
         ty::Bool => true,
fmt: checked 6344 files
Bootstrap failed while executing `test --stage 0 src/tools/tidy tidyselftest --extra-checks=py,cpp,js,spellcheck`
Build completed unsuccessfully in 0:00:45
  local time: Sun Aug 31 04:11:39 UTC 2025

@tgross35
Copy link
Contributor

tgross35 commented Aug 31, 2025

Please make the PR description a brief summary of what is fixed, rather than just crosslinking an issue (so we don't need to follow it for context, GH doesn't even link it). Also the commit message only says "Update layout.rs", ideally it should be the same as the description.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could really use a regression test to demonstrate that the fix indeed fixes the reported issue

View changes since this review

@xizheyin xizheyin changed the title Fixes #145248 Detect and optimize SIMD-like struct Aug 31, 2025
Comment on lines +582 to +587
if !matches!(crate_name_str,
"core" | "std" | "alloc" | "proc_macro" | "test" |
"rustc_std_workspace_core" | "rustc_std_workspace_alloc" |
"compiler_builtins" | "libc" | "unwind" | "panic_abort" |
"panic_unwind" | "adler2" | "object" | "memchr"
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do these get special treatment?

Name matching specific ecosystem crates to give them special treatment is very hazardous. Occasionally we need to match std crates, for which we have the list at

pub const STDLIB_STABLE_CRATES: &[Symbol] = &[sym::std, sym::core, sym::alloc, sym::proc_macro];
, but I can't think of a reason it would be needed here.

Comment on lines +591 to +597
if let ty::Array(element_ty, array_len) = field_ty.kind() {
if is_simd_friendly_element_type(*element_ty) {
if let Some(len) = extract_const_value(cx, ty, *array_len)?
.try_to_target_usize(tcx)
{
// Common SIMD sizes: 4, 8, 16 (conservative range)
if matches!(len, 4 | 8 | 16) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a let chain to flatten this nesting.

@tgross35
Copy link
Contributor

If I am understanding this correctly, it looks like it is changing arrays to SIMD vectors if they match a reasonable size/align? This probably makes sense in some cases, but doing it in layout seems way too early: things like ABI calculation rely on the computed layout, saying we have a SIMD type when we don't is probably going to cause confusion somewhere.

Tbh I'm not even certain this is an optimization we should be trying to do on the Rust side or if it's something LLVM has enough information for, what does the IR look like?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Very bad SIMD code generation for simple Euler integration
6 participants