Skip to content
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

AIX: use align 8 for byval parameter #134396

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

mustartt
Copy link
Contributor

@mustartt mustartt commented Dec 16, 2024

On AIX, byval pointer arguments are aligned to 8 bytes based on the 64bit register size. For example, the C callee https://godbolt.org/z/5f4vnG6bh will expect the following argument.

ptr nocapture noundef readonly byval(%struct.TwoU64s) align 8 %0

This case is captured by run-make/extern-fn-explicit-align

@rustbot
Copy link
Collaborator

rustbot commented Dec 16, 2024

r? @Nadrieril

rustbot has assigned @Nadrieril.
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 Dec 16, 2024
@mustartt mustartt changed the title use natural align 4 for byval args AIX: use natural align 4 for byval args Dec 16, 2024
@jieyouxu jieyouxu added the O-aix OS: Big Blue's Advanced Interactive eXecutive.. label Dec 16, 2024
Copy link

@gilamn5tr gilamn5tr left a comment

Choose a reason for hiding this comment

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

Looks good.

@workingjubilee
Copy link
Member

...wait, so on AIX, loads from the stack aren't aligned to the size of the type? whee...

@mustartt
Copy link
Contributor Author

mustartt commented Dec 17, 2024

...wait, so on AIX, loads from the stack aren't aligned to the size of the type? whee...

Not exactly the case. That's just how we are representing it in the IR. In reality, it almost always 16 byte aligned.

@workingjubilee
Copy link
Member

??? what is the actuality then?

@hubert-reinterpretcast
Copy link

hubert-reinterpretcast commented Dec 17, 2024

On AIX, byval pointer arguments have natural alignment of 4 bytes.

This sounds like a statement for 32-bit mode. TMK, in 64-bit mode, Clang produces 8-byte alignment.
Additionally, 16-byte alignment is used when the argument type involves a non-packed PPC vector.

@mustartt mustartt force-pushed the byval-pointer-natural-alignment branch from 51dcfae to 7bfcddf Compare December 17, 2024 23:37
@mustartt mustartt changed the title AIX: use natural align 4 for byval args AIX: use natural align 8 for byval args Dec 17, 2024
@mustartt mustartt changed the title AIX: use natural align 8 for byval args AIX: use align 8 for byval parameter Dec 17, 2024
@mustartt
Copy link
Contributor Author

AIX, loads from the stack aren't aligned to the size of the type?

For input values of the parameter, yes, the loads from the stack can be not aligned to the type. But the function local have to have proper alignment.

That's just how we are representing it in the IR.

We do not (at the assembly level) pass pointers to the callee. The incoming parameter is represented as a pointer in the IR (associated to register-sized/aligned memory when not stored in the registers themselves). That is: The alignment is associated with the size of registers (except where a parameter type has an non-packed vector in it).

@hubert-reinterpretcast
Copy link

But the function local have to have proper alignment.

@mustartt, Clang creates the function local separately, does the Rust front-end do so as well?

@mustartt
Copy link
Contributor Author

But the function local have to have proper alignment.

@mustartt, Clang creates the function local separately, does the Rust front-end do so as well?

Yes, the function locals will have the type's alignment. For example:

define i64 @rust_func(ptr byval([16 x i8]) align 8 %0) unnamed_addr #0 personality ptr @rust_eh_personality {
  start:
    %val = alloca [16 x i8], align 16
    call void @llvm.memcpy.p0.p0.i64(ptr align 16 %val, ptr align 8 %0, i64 16, i1 false)
  ...
}

@@ -58,8 +58,10 @@ where

// The AIX ABI expect byval for aggregates
// See https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/Targets/PPC.cpp.
// The incoming parameter is represented as a pointer in the IR,
// the alignment is associated with the size of the register. (align 8 for 64bit)

Choose a reason for hiding this comment

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

Should there be a TODO regarding PowerPC SIMD vector types (https://doc.rust-lang.org/core/arch/powerpc64/index.html)?

Choose a reason for hiding this comment

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

Probably. Its 16 for vector types?

Choose a reason for hiding this comment

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

Probably. Its 16 for vector types?

It's 16 for vector types and aggregates with (non-packed) vector subobjects. There seems to be a bug in Clang for the packed cases.

@Nadrieril
Copy link
Member

I know nothing about this :)
r? compiler

@rustbot rustbot assigned wesleywiser and unassigned Nadrieril Dec 21, 2024
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

This looks correct to me. Let's cc the AIX target maintainers though: @daltenty @gilamn5tr

@wesleywiser
Copy link
Member

One more friendly reminder to the target maintainers to see if they spot an issue with this before merging: @daltenty @gilamn5tr

@gilamn5tr
Copy link

@wesleywiser We're happy with it for now. LGTM!

@wesleywiser
Copy link
Member

Thanks for taking a look @gilamn5tr!

@bors r+

@bors
Copy link
Contributor

bors commented Jan 21, 2025

📌 Commit 7bfcddf has been approved by wesleywiser

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 21, 2025
Copy link
Contributor

@daltenty daltenty left a comment

Choose a reason for hiding this comment

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

Confirming this looks good from the AIX target perspective

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2025
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#133372 (Refactor dyn-compatibility error and suggestions)
 - rust-lang#134396 (AIX: use align 8 for byval parameter)
 - rust-lang#135156 (Make our `DIFlags` match `LLVMDIFlags` in the LLVM-C API)
 - rust-lang#135816 (Use `structurally_normalize` instead of manual `normalizes-to` goals in alias relate errors)
 - rust-lang#135823 (make UI tests that use `--test` work on panic=abort targets)
 - rust-lang#135850 (Update the `wasm-component-ld` tool)
 - rust-lang#135858 (rustdoc: Finalize dyn compatibility renaming)
 - rust-lang#135866 (Don't pick `T: FnPtr` nested goals as the leaf goal in diagnostics for new solver)
 - rust-lang#135874 (Enforce that all spans are lowered in ast lowering)
 - rust-lang#135875 (Remove `Copy` bound from `enter_forall`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit df01040 into rust-lang:master Jan 22, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-aix OS: Big Blue's Advanced Interactive eXecutive.. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

10 participants