Skip to content

Conversation

RalfJung
Copy link
Member

This provides a more informative warning when someone manually sets +soft-float on s390x.

@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 Sep 10, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 10, 2025

r? @davidtwco

rustbot has assigned @davidtwco.
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

@RalfJung
Copy link
Member Author

RalfJung commented Sep 10, 2025

The LLVM source code has some very odd-looking code that looks a bit like different registers are used for floats depending on the "vector" target feature? I hope that's not ABI-relevant in some way?

    if (Subtarget.hasVector()) {
      addRegisterClass(MVT::f16, &SystemZ::VR16BitRegClass);
      addRegisterClass(MVT::f32, &SystemZ::VR32BitRegClass);
      addRegisterClass(MVT::f64, &SystemZ::VR64BitRegClass);
    } else {
      addRegisterClass(MVT::f16, &SystemZ::FP16BitRegClass);
      addRegisterClass(MVT::f32, &SystemZ::FP32BitRegClass);
      addRegisterClass(MVT::f64, &SystemZ::FP64BitRegClass);
    }

Cc @uweigand @taiki-e

(I don't really understand this LLVM code, but this is the code where for other targets I found interesting ABI-relevant target features that we had missed before so I now always check it when adding a new target to this list.)

EDIT: I found a note online saying that the vector registers overlap the float registers, so maybe for ABI questions then both are equivalent and that's why this is fine? However there are apparently more vector registers than float registers so it'd still be important that the "higher" registers are not used to pass float arguments.

@rust-log-analyzer

This comment has been minimized.

@uweigand
Copy link
Contributor

The LLVM source code has some very odd-looking code that looks a bit like different registers are used for floats depending on the "vector" target feature? I hope that's not ABI-relevant in some way?

No, that's not ABI relevant. This is only about registers that may be used to hold FP values inside a function, it doesn't define the ABI. Note that the VR... classes are a superset of the FP... classes, so this simply says that if the vector feature is available, we can use 32 registers to hold FP values (the original 16 plus 16 more). For ABI purposes, FP values are always passed in the same set of registers (some of the original 16).

@RalfJung
Copy link
Member Author

Great, thanks for confirming!

Comment on lines 1184 to 1186
// The "vector" target feature does cause floats to be put into vector
// registers, but those overlap the float registers so it makes no
// difference for the ABI.
Copy link
Member

Choose a reason for hiding this comment

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

Based on what @uweigand said, the "vector" feature's effects on float handling is entirely intra-function as opposed to cross-function and thus isn't ABI-relevant? Perhaps it should instead be

Suggested change
// The "vector" target feature does cause floats to be put into vector
// registers, but those overlap the float registers so it makes no
// difference for the ABI.
// The "vector" target feature does not affect the ABI for floats,
// and vector registers overlap the float registers anyways.

Copy link
Member Author

Choose a reason for hiding this comment

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

From an LLVM perspective it seems like it does not affect ABI because they overlap -- LLVM will emit code for floats that references the vector registers (at least conceptually, not sure if that makes a difference in the machine code), but because of the overlap, that doesn't make a difference.

Copy link
Member

@workingjubilee workingjubilee Sep 10, 2025

Choose a reason for hiding this comment

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

I believe, based on what @uweigand just said, that there's 16 extra vector registers with the "vector" feature, and those can also be used to hold floats. That is... let's give ourselves 32 logical registers:

0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
FP FP FP FP FP FP FP FP FP FP FP FP FP FP FP FP - - - - - - - - - - - - - - - -
VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR VR

So for instructions that are designed to address the old floating-point range, they likely have 4 bits for addressing in their opcode and still only work on the "bottom halves" of the vector registers that occupy r0 through r15. But new vector operations, including "upgraded" versions of old float instructions, can still do float operations, and thus can manipulate r16 through r31 as if they are floating point registers.

However, it still breaks ABI if you use r16 through r31 to pass floats to functions that aren't built with an awareness of these registers. So we simply don't! Easy choice, there.

Copy link
Member

Choose a reason for hiding this comment

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

Hm I think you already understand this and I just was concerned about wording :^)

Copy link
Member Author

Choose a reason for hiding this comment

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

My point is that I think LLVM will actually use the new instructions to work with floats when "vector" is enabled, so it may look like it passes arguments in different registers (v0 instead of f0), but actually it does not. That's the confusion I wanted to explain with the comment.

(Apparently only registers 0, 2, 4, 6 are ever used for the calling convention.)

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

that mildly silly nit aside:

View changes since this review

@@ -8,6 +8,7 @@
// FIXME: +soft-float itself doesn't set -vector
//@[z13_soft_float] compile-flags: --target s390x-unknown-linux-gnu -C target-cpu=z13 -C target-feature=-vector,+soft-float
//@[z13_soft_float] needs-llvm-components: systemz
//[z13_soft_float]~? WARN must be disabled to ensure that the ABI of the current target can be implemented correctly
Copy link
Member

Choose a reason for hiding this comment

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

...we really should get a more consistent annotation syntax here...

@workingjubilee
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Sep 10, 2025

📌 Commit 3b21783 has been approved by workingjubilee

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 Sep 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

7 participants