[x86] LLVM silently switches to the soft-float ABI if FPU is disabled on hard-float target

[RalfJung]

Here's a godbolt reproducer: https://godbolt.org/z/GGfTEWcPT
This is basically the X86 version of #110383.

-mno-sse -mno-x87 switches to the softfloat ABI. I would expect this to only happen when I set -msoft-float. (Though strangely, that flag on its own does not seem to change the ABI at all?)

Strangely, passing just -mno-sse to clang behaves as expected: it shows an error. But adding -mno-x87 (which does not seem to exist for GCC) makes the error go away.

I am a Rust person so I care mostly about the behavior of the backend here, and less about the clang frontend. The underlying issue is that if Rust sets -x87 for the target features in createTargetMachine we should get an error since the registers for the ABI are missing; +soft-float,-x87 should work because we requested soft-float and then it's fine for the registers to be missing.

[llvmbot]

@llvm/issue-subscribers-backend-x86

Author: Ralf Jung (RalfJung)

Here's a godbolt reproducer: https://godbolt.org/z/GGfTEWcPT This is basically the X86 version of https://github.com//issues/110383.

-mno-sse -mno-x87 switches to the softfloat ABI. I would expect this to only happen when I set -msoft-float. (Though strangely, that flag on its own does not seem to change the ABI at all?)

Strangely, passing just -mno-sse to clang behaves as expected: it shows an error. But adding -mno-x87 (which does not seem to exist for GCC) makes the error go away.

I am a Rust person so I care mostly about the behavior of the backend here, and less about the clang frontend. The underlying issue is that if Rust sets -x87 for the target features in createTargetMachine we should get an error since the registers for the ABI are missing; +soft-float,-x87 should work because we requested soft-float and then it's fine for the registers to be missing.

[phoebewang]

GCC has option -mno-80387, but it gives error even with -msoft-float. The ABI check should happen in the front end, so I think Rust should give error about this if you concern it.

[RalfJung]

That requires duplicating the ABI logic in the frontend. It is hard to figure out which flags might make LLVM silently change ABI, and this might even change on LLVM updates. So doing this in the frontend is an endless game of wack-a-mole.

[phoebewang]

Silently changing ABI is allowed in some cases, e.g., https://godbolt.org/z/dEs4cjGYr

[RalfJung]

That looks like a GCC bug to me, given that it generally strives to clearly tell when the requested ABI cannot be realized.

[phoebewang]

It is a feature, see #50840

[RalfJung]

-mno-fp-ret asks for an ABI change. That's very different than -mno-x87 silently changing the ABI.

[phoebewang]

No sure GCC's implementation, but we simply do an alias in Clang. Anyway, the option differentiation is a FE job instead of backend.

[RalfJung]

As I already said (but you have not replied to), that requires duplicating a bunch if logic. The backend should at least expose this information to the frontend; it would be silly for every frontend to duplicate that.

[phoebewang]

The frontend has already had a lot of knowledge about ABI information, see clang/lib/CodeGen/Targets/X86.cpp. Sinking them in backend is not efficient or even possible in some cases. So I suggest frontend to check it following the precedent.
In above example, the backend cannot emit error for one option but not for another whitout extra options in the IR. And consider backend is not good at diagnosis (e.g., precise position), it's not worth to do so.

[RalfJung]

Other frontends have way less ABI information, e.g. the Rust compiler. It would be a huge waste of effort to duplicate this in all frontends.

And consider backend is not good at diagnosis (e.g., precise position), it's not worth to do so.

Yeah backend errors currently do not look great, but that can be fixed by having some way for the backend to give error information to the frontend rather than printing the error itself.

Also, even if the frontend has its own checks, it's always good to have a second line of defense -- if something slips through the frontend checks, it's good for the backend to catch that. Right now the same logic is implemented twice and if there's an accidental mismatch it leads to very subtle surprising behavior. It would be much better to instead have a backend error in that case, even if a backend error looks less nice than a frontend error.

[phoebewang]

Also, even if the frontend has its own checks, it's always good to have a second line of defense -- if something slips through the frontend checks, it's good for the backend to catch that. Right now the same logic is implemented twice and if there's an accidental mismatch it leads to very subtle surprising behavior. It would be much better to instead have a backend error in that case, even if a backend error looks less nice than a frontend error.

I agree with the idea of second line defense and I actually suggested to do similar work with an ABI verifier, see #100757 (comment)

Here you go :)

[RalfJung]

@phoebewang thanks for filing that PR. :) Seems like unfortunately reviewers do not agree with the approach.

For RISCV, the ABI logic in RISCVISelLowering prints a warning when an ABI was requested but not all features for that ABI are available. Maybe a similar warning could be added to the x86 file? On x86 the ABI is requested via the soft-float feature rather than via an explicit ABI field, which is an unfortunate inconsistency, but I'd say that's just a historic accident and does not justify being more or less careful about ABI issues on these targets: if soft-float is not set, that is requesting the hardfloat ABI, and therefore the x87 feature is required (and on x86-64, also the sse2 feature).

[phoebewang]

@phoebewang thanks for filing that PR. :) Seems like unfortunately reviewers do not agree with the approach.

For RISCV, the ABI logic in RISCVISelLowering prints a warning when an ABI was requested but not all features for that ABI are available. Maybe a similar warning could be added to the x86 file? On x86 the ABI is requested via the soft-float feature rather than via an explicit ABI field, which is an unfortunate inconsistency, but I'd say that's just a historic accident and does not justify being more or less careful about ABI issues on these targets: if soft-float is not set, that is requesting the hardfloat ABI, and therefore the x87 feature is required (and on x86-64, also the sse2 feature).

We check the feature when lowering return on X86 . The problem is when both x87 and sse are disabled, the FP type won't be assigned to a valid register class. So the type will be transformed into integer by default and no ABI issue then when lowering return. It is the general handling of FP types that provided by backend framework when target doesn't provide FP unit.

That says, backend doesn't distinguish soft-float feature with no FP features enabled. Either it takes both identical or UB if soft-float is not set. Either way, it a FE concerns the difference, it should emit diagnosis itself.

[RalfJung]

I'm sorry I wasn't able to fully understand this. But given that RISC-V handles this properly, emitting a warning when requesting an invalid combination (hardfloat ABI but no FP unit), I think X86 should do the same. Maybe we have to first introduce a more explicit notion of "requesting hardfloat ABI" for X86 if you are not happy with using "absence of soft-float" as indication for requesting a hardfloat ABI?

or UB if soft-float is not set

Where can I find the documentation on UB like this? Clearly it is very important to precisely document all the UB, otherwise how are frontends supposed to know that there is UB here.

[phoebewang]

I don't see the concept of hardfloat ABI in LLVM. Here is the code I mentioned that transforms FP into integer type:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/TargetLoweringBase.cpp#L1355

And X86 checks it here:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86ISelLoweringCall.cpp#L788

The check happens after type been transformed, and for an integer type, it's correct to not error out here. As you can see, the framework doesn't care about ABI when target does not have native FP support.

I'm not sure it's UB, but ABI related UB seldom been documented. I think one reason is it's always target specific and been handled well by CFE. It's not convenient for other FE to do them all, so I'm in favor of modulize that part as much as possible and share among different FEs. Of course, we can do more strict verifier like I proposed.

[RalfJung]

I don't see the concept of hardfloat ABI in LLVM.

It seems to be something each target has to do on its own. For RISCV, this happens here:

llvm-project/llvm/lib/Target/RISCV/RISCVISelLowering.cpp

Line 89 in ecd59f8

errs() << "Hard-float 'f' ABI can't be used for a target that "

IMO ideally all targets would work something like that: the user explicitly requests an ABI, and then LLVM at least warns if somehow the ABI cannot be implemented.

The X86 check you mention approximates this somewhat, though it fails to capture the case of an x87 register return occurring when x87 is disabled.