powerpc: warn against incorrect values for ABI-relevant target features

[RalfJung]

View all comments

This fills in #131799 for PowerPC. Based on this comment by @beetrees, the relevant target features are "hard-float" and "spe". I confirmed this by looking at the LLVM sources:

  // Set up the register classes.
  addRegisterClass(MVT::i32, &PPC::GPRCRegClass);
  if (!useSoftFloat()) {
    if (hasSPE()) {
      addRegisterClass(MVT::f32, &PPC::GPRCRegClass);
      // EFPU2 APU only supports f32
      if (!Subtarget.hasEFPU2())
        addRegisterClass(MVT::f64, &PPC::SPERCRegClass);
    } else {
      addRegisterClass(MVT::f32, &PPC::F4RCRegClass);
      addRegisterClass(MVT::f64, &PPC::F8RCRegClass);
    }
  }

(this is in llvm/lib/Target/PowerPC/PPCISelLowering.cpp)

So, we make rustc emit a warning indicating the ABI compatibility issues if "spe" or hard-float" gets toggled. The plan is to eventually make this a hard error.

I also found this code there, in the handling for "altivec", that look like they are enabling more registers to be used for the ABI, but maybe I am missing a subtle difference in these addRegisterClass calls:

      if (Subtarget.hasP8Vector())
        addRegisterClass(MVT::f32, &PPC::VSSRCRegClass);

      addRegisterClass(MVT::f64, &PPC::VSFRCRegClass);

      addRegisterClass(MVT::v4i32, &PPC::VSRCRegClass);
      addRegisterClass(MVT::v4f32, &PPC::VSRCRegClass);
      addRegisterClass(MVT::v2f64, &PPC::VSRCRegClass);

Cc @nikic for help with interpreting this LLVM code.

Cc @Gelbpunkt @famfo @neuschaefer as maintainers of affected targets

[rustbot]

r? @chenyukang

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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 73 candidates
• Random selection from 19 candidates

[Gelbpunkt]

So, we make rustc emit a warning indicating the ABI compatibility issues if "spe" or hard-float" gets toggled. The plan is to eventually make this a hard error.

This is correct to my knowledge, the SPE instructions perform float-point math on the integer registers.

However I'm wondering whether this change will break the SPE targets since to my understanding this enforces hard-float to be enabled and spe to not be enabled for any of the 32-bit PowerPC targets, which includes the existing SPE targets?

[Gelbpunkt]

Oh, well, now that I'm looking at the SPE target definitions, they don't even seem to enable SPE in LLVM, so they're probably already broken as-is

[RalfJung]

Oh I didn't realize we have SPE targets.

But yeah, looking at those targets... they set -mspe for the linker, but they don't seem to tell LLVM that this should be using SPE? How dos that make any sense? Does LLVM use the target string to figure out the ABI? From the LLVM code, it doesn't look like it.
Cc @BKPepe @biabbas @hax0kartik (you are listed as maintainers for SPE targets)

[RalfJung]

This is correct to my knowledge, the SPE instructions perform float-point math on the integer registers.

That wouldn't matter for the ABI. But I guess they will then also use integer registers for passing float arguments / return values, and that makes them ABI-incompatible with non-SPE targets?

[Gelbpunkt]

That wouldn't matter for the ABI. But I guess they will then also use integer registers for passing float arguments / return values, and that makes them ABI-incompatible with non-SPE targets?

Yes, sorry for not being clear on that in my previous reply.

[Gelbpunkt]

See also https://www.nxp.com/docs/en/reference-manual/E500ABIUG.pdf

The following algorithm specifies where argument data is passed for the C language [...]

SIMPLE_ARG: A SIMPLE_ARG is one of the following:
— One of the simple integer types no more than 32 bits wide (char, short, int, long, enum)
— A single-precision float
— A pointer to an object of any type
— A struct, union, or long double, any of which shall be treated as a pointer to the
object, or to a copy of the object where necessary to enforce call-by-value
semantics. Only if the caller can ascertain that the object is constant can it pass
a pointer to the object itself.
If gr>10, go to OTHER. Otherwise, load the argument value into general register gr,
set gr to gr+1, and go to SCAN. Values shorter than 32 bits are sign-extended or
zero-extended, depending on whether they are signed or unsigned.

64-bit floats are passed in 2 GPRs.

[chenyukang]

@rustbot reroll

[mu001999]

@rustbot reroll

[petrochenkov]

Waiting for the pinged people to respond (for 1-2 weeks max, I guess).
@rustbot blocked

[hax0kartik]

Oh I didn't realize we have SPE targets.

But yeah, looking at those targets... they set -mspe for the linker, but they don't seem to tell LLVM that this should be using SPE? How dos that make any sense? Does LLVM use the target string to figure out the ABI? From the LLVM code, it doesn't look like it. Cc @BKPepe @biabbas @hax0kartik (you are listed as maintainers for SPE targets)

Atleast, for vxWorks, I can confirm that this is an error in the spec file. It needs to pass a -mspe / -mcpu flag(-mcpu=8548) to llvm to have it generate the spe instructions. Thanks for pointing this out.

[RalfJung]

So, this target is more than 6 years old, and it never actually generated correct machine code...?

Clearly we can just remove it then as it cannot have any users? Or am I missing something? Does normal non-SPE code mostly work on SPE hardware and that's why nobody noticed?

[hax0kartik]

So, this target is more than 6 years old, and it never actually generated correct machine code...?

Clearly we can just remove it then as it cannot have any users? Or am I missing something? Does normal non-SPE code mostly work on SPE hardware and that's why nobody noticed?

I believe we do not have any customer currently who uses rust for ppc arch and that too for SPE. Nonetheless, we would still like to keep support for this. I must agree that the target hasn’t been well tested, if tested at all.

Normal ppc float instructions do not work on spe.

Pl give 1-2 days to work on a fix for this. Thanks.

[taiki-e]

While users of spe targets do exist (as of at least a year ago), in practice they always use -C target-cpu, so they rarely encounter this kind of problem.

I previously worked on getting these targets to work without -C target-cpu (even though I'm not an actual user), but I haven't had much time lately and only created a patch for the spe feature issue (taiki-e@8185fbc), without submitting it (UPDATE: submitted #157137).

• rustc_target: Add msync target feature and enable it on powerpcspe targets #137860
• PowerPC SPE targets don't have spe target feature enabled #138960

[RalfJung]

@taiki-e so there are -Ctarget-cpu values that imply spe?

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[RalfJung]

This should be ready then. :)
@rustbot ready

[petrochenkov]

@bors r+

[rust-bors]

📌 Commit 591d5da has been approved by petrochenkov

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 5. This pull request will be tested once the tree is reopened.