Open RalfJung opened 1 month ago
@llvm/issue-subscribers-backend-x86
Author: Ralf Jung (RalfJung)
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.
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.
Silently changing ABI is allowed in some cases, e.g., https://godbolt.org/z/dEs4cjGYr
That looks like a GCC bug to me, given that it generally strives to clearly tell when the requested ABI cannot be realized.
It is a feature, see https://github.com/llvm/llvm-project/issues/50840
-mno-fp-ret asks for an ABI change. That's very different than -mno-x87 silently changing the ABI.
No sure GCC's implementation, but we simply do an alias in Clang. Anyway, the option differentiation is a FE job instead of backend.
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.
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.
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.
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 https://github.com/llvm/llvm-project/pull/100757#discussion_r1693738922
Here you go :)
Here's a godbolt reproducer: https://godbolt.org/z/GGfTEWcPT This is basically the X86 version of https://github.com/llvm/llvm-project/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 increateTargetMachine
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.