Fix argument ABI for overaligned structs on ppc64le #122781

nikic · 2024-03-20T17:00:32Z

When passing a 16 (or higher) aligned struct by value on ppc64le, it needs to be passed as an array of i128 rather than an array of i64. This will force the use of an even starting doubleword.

For the case of a 16 byte struct with alignment 16 it is important that [1 x i128] is used instead of i128 -- apparently, the latter will get treated similarly to [2 x i64], not exhibiting the correct ABI. Add a force_array flag to Uniform to support this.

The relevant clang code can be found here:
https://github.com/llvm/llvm-project/blob/fe2119a7b08b6e468b2a67768904ea85b1bf0a45/clang/lib/CodeGen/Targets/PPC.cpp#L878-L884
https://github.com/llvm/llvm-project/blob/fe2119a7b08b6e468b2a67768904ea85b1bf0a45/clang/lib/CodeGen/Targets/PPC.cpp#L780-L784

I think the corresponding psABI wording is this:

Fixed size aggregates and unions passed by value are mapped to as
many doublewords of the parameter save area as the value uses in
memory. Aggregrates and unions are aligned according to their
alignment requirements. This may result in doublewords being
skipped for alignment.

In particular the last sentence. Though I didn't find any wording for Clang's behavior of clamping the alignment to 16.

Fixes #122767.

r? @cuviper

cuviper · 2024-03-20T17:25:48Z

Do we also need to force array type for homogeneous aggregates like clang does here?
https://github.com/llvm/llvm-project/blob/b20360abeb3a80281dc082f1e093abd13cb1ee4c/clang/lib/CodeGen/Targets/PPC.cpp#L855-L863

cuviper · 2024-03-20T17:28:01Z

compiler/rustc_target/src/abi/call/mod.rs

@@ -255,11 +255,14 @@ pub struct Uniform {
    ///   can be shorter, i.e., `{ i64, i64, i32 }` for
    ///   64-bit integers with a total size of 20 bytes.
    pub total: Size,
+
+    /// Force the use of an array, even if there is only a single element.
+    pub force_array: bool,


Given that so many places are just passing false, it might be nicer to have a fn new(unit, total) for non-array and another constructor for arrays.

This is done now. All uses switched to either Uniform::new or Uniform::consecutive.

erikdesjardins · 2024-03-20T22:16:02Z

compiler/rustc_codegen_llvm/src/abi.rs

            // Simplify to a single unit when there is no prefix and size <= unit size
-            if self.rest.total <= self.rest.unit.size {
+            if self.rest.total <= self.rest.unit.size && !self.rest.force_array {
                return rest_ll_unit;
            }


Perhaps we should assert if force_array is true and there's a prefix, since we'll generate a struct in that case.

bjorn3 · 2024-03-20T22:47:14Z

compiler/rustc_codegen_llvm/src/abi.rs

@@ -141,7 +141,7 @@ impl LlvmType for CastTarget {

        if self.prefix.iter().all(|x| x.is_none()) {
            // Simplify to a single unit when there is no prefix and size <= unit size
-            if self.rest.total <= self.rest.unit.size {
+            if self.rest.total <= self.rest.unit.size && !self.rest.force_array {


Could you please add an assertion that force_array is not true in

rust/compiler/rustc_codegen_cranelift/src/abi/pass_mode.rs

Line 41 in e3df96c

fn cast_target_to_abi_params(cast: &CastTarget) -> SmallVec<[AbiParam; 2]> {

Cranelift IR doesn't jave a way to represent array types.

After having looked into this more, I suspect that the cranelift lowering actually is not quite correct and you'll have to handle the new is_consecutive flag somehow. Or at least, I don't see how your current code would ensure consecutive allocation. Note that this part applies to not just ppc64le, but also to aarch64.

abi-cafe doesn't show any issues on aarch64 last time I checked.

nikic · 2024-03-21T10:02:03Z

Do we also need to force array type for homogeneous aggregates like clang does here? https://github.com/llvm/llvm-project/blob/b20360abeb3a80281dc082f1e093abd13cb1ee4c/clang/lib/CodeGen/Targets/PPC.cpp#L855-L863

Good question... I've been trying to figure out when exactly iN vs [1 x iN] makes a difference in terms of ABI.

It looks like the relevant TLI hook is functionArgumentNeedsConsecutiveRegisters(), which is overridden by ARM, AArch64 and PowerPC. The primary purpose of it is to ensure that either all the values get allocated to registers, or everything is spilled to the stack in packed representation, which is usually required for homogeneous aggregates.

Now, for single-element arrays, this usually wouldn't matter -- except in the case where the single element itself needs to be split across multiple registers, as is the case here with i128 on a target that has 64-bit registers. PowerPC has some additional magic in that this also affects the register passing ABI, but even without that it would affect whether it's allowed to spill only one of the registers to stack.

For that reason, I think that this doesn't matter for homogenous aggregates, because they are typically only defined on float/vector members that can be passed in one FPR register -- though maybe the upcoming f128 type could change something here?

nikic · 2024-03-21T15:56:29Z

Okay, I've reworked how this is done now. Quoting the commit comment:

The actual ABI implication here is that in some cases the values
are required to be "consecutive", i.e. must either all be passed
in registers or all on stack (without padding).

Adjust the code to either use Uniform::new() or Uniform::consecutive()
depending on which behavior is needed.

Then, when lowering this in LLVM, skip the [1 x i128] to i128
simplification if is_consecutive is set. i128 is the only case
I'm aware of where this is problematic right now. If we find
other cases, we can extend this (either based on target information
or possibly just by not simplifying for is_consecutive entirely).

That way, rustc_target just describes the required ABI, and the question of how this is achieved in terms of LLVM types is punted to rustc_codegen_llvm.

cuviper

There are a few more struct initializers that should now be Uniform::new, in nvptx64, riscv, sparc, and sparc64.

Otherwise, LGTM!

bors · 2024-04-03T23:03:26Z

☔ The latest upstream changes (presumably #123429) made this pull request unmergeable. Please resolve the merge conflicts.

When passing a 16 (or higher) aligned struct by value on ppc64le, it needs to be passed as an array of `i128` rather than an array of `i64`. This will force the use of an even starting register. For the case of a 16 byte struct with alignment 16 it is important that `[1 x i128]` is used instead of `i128` -- apparently, the latter will get treated similarly to `[2 x i64]`, not exhibiting the correct ABI. Add a `force_array` flag to `Uniform` to support this. The relevant clang code can be found here: https://github.com/llvm/llvm-project/blob/fe2119a7b08b6e468b2a67768904ea85b1bf0a45/clang/lib/CodeGen/Targets/PPC.cpp#L878-L884 https://github.com/llvm/llvm-project/blob/fe2119a7b08b6e468b2a67768904ea85b1bf0a45/clang/lib/CodeGen/Targets/PPC.cpp#L780-L784 I think the corresponding psABI wording is this: > Fixed size aggregates and unions passed by value are mapped to as > many doublewords of the parameter save area as the value uses in > memory. Aggregrates and unions are aligned according to their > alignment requirements. This may result in doublewords being > skipped for alignment. In particular the last sentence. Fixes rust-lang#122767.

The actual ABI implication here is that in some cases the values are required to be "consecutive", i.e. must either all be passed in registers or all on stack (without padding). Adjust the code to either use Uniform::new() or Uniform::consecutive() depending on which behavior is needed. Then, when lowering this in LLVM, skip the [1 x i128] to i128 simplification if is_consecutive is set. i128 is the only case I'm aware of where this is problematic right now. If we find other cases, we can extend this (either based on target information or possibly just by not simplifying for is_consecutive entirely).

nikic · 2024-04-08T02:42:18Z

There are a few more struct initializers that should now be Uniform::new, in nvptx64, riscv, sparc, and sparc64.

Otherwise, LGTM!

Done & rebased!

cuviper · 2024-04-08T17:18:57Z

@bors r+

bors · 2024-04-08T17:19:00Z

📌 Commit 1b7342b has been approved by cuviper

It is now in the queue for this repository.

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#122781 (Fix argument ABI for overaligned structs on ppc64le) - rust-lang#123367 (Safe Transmute: Compute transmutability from `rustc_target::abi::Layout`) - rust-lang#123518 (Fix `ByMove` coroutine-closure shim (for 2021 precise closure capturing behavior)) - rust-lang#123547 (bootstrap: remove unused pub fns) - rust-lang#123564 (Don't emit divide-by-zero panic paths in `StepBy::len`) - rust-lang#123578 (Restore `pred_known_to_hold_modulo_regions`) - rust-lang#123591 (Remove unnecessary cast from `LLVMRustGetInstrProfIncrementIntrinsic`) - rust-lang#123632 (parser: reduce visibility of unnecessary public `UnmatchedDelim`) - rust-lang#123635 (CFI: Fix ICE in KCFI non-associated function pointers) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#122781 - nikic:ppc-abi-fix, r=cuviper Fix argument ABI for overaligned structs on ppc64le When passing a 16 (or higher) aligned struct by value on ppc64le, it needs to be passed as an array of `i128` rather than an array of `i64`. This will force the use of an even starting doubleword. For the case of a 16 byte struct with alignment 16 it is important that `[1 x i128]` is used instead of `i128` -- apparently, the latter will get treated similarly to `[2 x i64]`, not exhibiting the correct ABI. Add a `force_array` flag to `Uniform` to support this. The relevant clang code can be found here: https://github.com/llvm/llvm-project/blob/fe2119a7b08b6e468b2a67768904ea85b1bf0a45/clang/lib/CodeGen/Targets/PPC.cpp#L878-L884 https://github.com/llvm/llvm-project/blob/fe2119a7b08b6e468b2a67768904ea85b1bf0a45/clang/lib/CodeGen/Targets/PPC.cpp#L780-L784 I think the corresponding psABI wording is this: > Fixed size aggregates and unions passed by value are mapped to as > many doublewords of the parameter save area as the value uses in > memory. Aggregrates and unions are aligned according to their > alignment requirements. This may result in doublewords being > skipped for alignment. In particular the last sentence. Though I didn't find any wording for Clang's behavior of clamping the alignment to 16. Fixes rust-lang#122767. r? `@cuviper`

rustbot assigned cuviper Mar 20, 2024

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 Mar 20, 2024

cuviper reviewed Mar 20, 2024

View reviewed changes

erikdesjardins reviewed Mar 20, 2024

View reviewed changes

bjorn3 reviewed Mar 20, 2024

View reviewed changes

cuviper reviewed Mar 28, 2024

View reviewed changes

nikic added 2 commits April 8, 2024 11:15

nikic force-pushed the ppc-abi-fix branch from bfd1ddc to 1b7342b Compare April 8, 2024 02:37

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 Apr 8, 2024

matthiaskrgr mentioned this pull request Apr 8, 2024

Rollup of 9 pull requests #123645

Merged

bors merged commit ecfc338 into rust-lang:master Apr 8, 2024
11 checks passed

rustbot added this to the 1.79.0 milestone Apr 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix argument ABI for overaligned structs on ppc64le #122781

Fix argument ABI for overaligned structs on ppc64le #122781

nikic commented Mar 20, 2024 •

edited

Loading

cuviper commented Mar 20, 2024

cuviper Mar 20, 2024

nikic Mar 21, 2024

erikdesjardins Mar 20, 2024

bjorn3 Mar 20, 2024 •

edited

Loading

nikic Mar 21, 2024

bjorn3 Mar 21, 2024

nikic commented Mar 21, 2024

nikic commented Mar 21, 2024

cuviper left a comment

bors commented Apr 3, 2024

nikic commented Apr 8, 2024

cuviper commented Apr 8, 2024

bors commented Apr 8, 2024

Fix argument ABI for overaligned structs on ppc64le #122781

Fix argument ABI for overaligned structs on ppc64le #122781

Conversation

nikic commented Mar 20, 2024 • edited Loading

cuviper commented Mar 20, 2024

cuviper Mar 20, 2024

Choose a reason for hiding this comment

nikic Mar 21, 2024

Choose a reason for hiding this comment

erikdesjardins Mar 20, 2024

Choose a reason for hiding this comment

bjorn3 Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

nikic Mar 21, 2024

Choose a reason for hiding this comment

bjorn3 Mar 21, 2024

Choose a reason for hiding this comment

nikic commented Mar 21, 2024

nikic commented Mar 21, 2024

cuviper left a comment

Choose a reason for hiding this comment

bors commented Apr 3, 2024

nikic commented Apr 8, 2024

cuviper commented Apr 8, 2024

bors commented Apr 8, 2024

nikic commented Mar 20, 2024 •

edited

Loading

bjorn3 Mar 20, 2024 •

edited

Loading