This is an archive of the discontinued LLVM Phabricator instance.

[llvm][RISC-V] Use floating-point ABI by default if possible
Needs ReviewPublic

Authored by occheung on Nov 15 2021, 5:45 PM.

Details

Reviewers
asb
Summary

This patch makes changes to the default ABI for RISC-V targets.

Before patch, only integer ABI (ilp32e, ilp32, lp64) can be selected by default (without passing ABIName in MCOptions).
After patch, ilp32f/d and lp64f/d can be selected if F/D extensions are detected.

Tests are updated by adding specifying target-abi if necessary.

Diff Detail

Event Timeline

occheung created this revision.Nov 15 2021, 5:45 PM
occheung requested review of this revision.Nov 15 2021, 5:45 PM

I believe this is the consensus in https://github.com/riscv-non-isa/riscv-toolchain-conventions/issues/13 (which should be linked to). Do you know if GCC is doing the same? Because even if a certain default makes more sense than another, it's best to be consistent between toolchains to avoid surprises.

I believe this is the consensus in https://github.com/riscv-non-isa/riscv-toolchain-conventions/issues/13 (which should be linked to). Do you know if GCC is doing the same? Because even if a certain default makes more sense than another, it's best to be consistent between toolchains to avoid surprises.

Isn't clang behavior determined by riscv::getRISCVABI in clang/lib/Driver/ToolChains/Arch/RISCV.cpp rather than this code?

All the preparatory changes to the RUN lines should probably be committed separately from the functional change. How did you update the tests; did you search for all that enabled f and d, or did you just look for ones that failed? Just wondering if there's any subtlety there about tests that appear to be unchanged but end up following slightly different code paths from what they intend to test.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
37

I'd put these two declarations the other way round so they match the canonical order; only the uses need to be in "reverse" order to ensure we check from most to least specific.

61–62

This comment isn't really correct any more, even with your added sentence; clearly you can't use an lp64 ABI on rv32, the whole point of it was stating that a soft-float ABI is the default.

I believe this is the consensus in https://github.com/riscv-non-isa/riscv-toolchain-conventions/issues/13 (which should be linked to). Do you know if GCC is doing the same? Because even if a certain default makes more sense than another, it's best to be consistent between toolchains to avoid surprises.

Isn't clang behavior determined by riscv::getRISCVABI in clang/lib/Driver/ToolChains/Arch/RISCV.cpp rather than this code?

Oh, indeed, that's still separate. I'm in two minds as to whether llc should default to soft-float or base it on the attributes (ignoring the F-without-D default for now). Being consistent with clang has its benefits, but normally you want llc to be simple and not have it start changing behaviour in other ways when you just add an attribute...

How did you update the tests; did you search for all that enabled f and d, or did you just look for ones that failed? Just wondering if there's any subtlety there about tests that appear to be unchanged but end up following slightly different code paths from what they intend to test.

I looked for the ones that failed without specifying abi and then added the -target-abi ilp32 / lp64 argument.
Regardless of what the intended test were, llc will use ilp32/lp64 as default abi before patch, so I don't think setting -target-abi as that intended default values (before patch) actually changed the test.

How did you update the tests; did you search for all that enabled f and d, or did you just look for ones that failed? Just wondering if there's any subtlety there about tests that appear to be unchanged but end up following slightly different code paths from what they intend to test.

I looked for the ones that failed without specifying abi and then added the -target-abi ilp32 / lp64 argument.
Regardless of what the intended test were, llc will use ilp32/lp64 as default abi before patch, so I don't think setting -target-abi as that intended default values (before patch) actually changed the test.

That's not what I'm concerned about; clearly those have no change in behaviour. It's the ones that _still_ use the computed default ABI that may now have changed for them but happen to not change the output; just because the output is the same doesn't mean the test is still testing the same thing. It's probably fine, but I'm just raising it as a potential concern to think about.

occheung updated this revision to Diff 387462.Nov 15 2021, 6:11 PM

Rebased to d7a8135.
I am unsure why the patching failed in the first place.

occheung updated this revision to Diff 387470.Nov 15 2021, 6:41 PM

Declared IsRVF before IsRVD.
Removed invalid ABI comment.

occheung updated this revision to Diff 387480.Nov 15 2021, 7:23 PM

Removed the unnecessarily added -target-abi ilp32/lp64 when neither F nor D extensions were included. (e.g. loop unroll test in the Transform subdirectory)

asb added a comment.Nov 17 2021, 7:11 AM

I've got no real objection to changing the default, but I just wondered if the current default was causing a problem for you?

I'd considered the clang-side default as user-facing and pretty important, but would have thought the LLVM tool defaults were less important. Even then, I can see the logic in aligning with what Clang does.

I've got no real objection to changing the default, but I just wondered if the current default was causing a problem for you?

It becomes an issue when we attempt to write LLVM IR with other platforms (e.g. llvmlite, inkwell, etc).
These libraries may not expose the ABI argument to their API, so when compiling with RISC-V hard-float targets, it will use the default integer ABI (ilp32/lp64).
Some other languages based on LLVM (e.g. Rust) may have selected the float ABI by their default.
It will cause ABI conflict when we need to link libraries built with different platforms (e.g. link dynamic libraries built with llvmlite to a binary built with Rust).

Well technically making a patch to these platforms would solve the issue (as my PR in llvmlite), but we will eventually have to apply the same patch to every application that didn't/haven't sort out the ABI argument.

That's not a convincing argument to me. If they only support one RISC-V ABI they can jut hardcode it. If they support multiple then they can expose it however they like.

(In the same way that they'll already have a hard-coded list of target features; it's really not a huge imposition to require them to also set the ABI at the same time)

asb added a comment.Nov 18 2021, 2:03 AM

I think anyone producing LLVM IR for RISC-V should explicitly set the ABI and of course any target features (-mattr for llc). The default rv32/rv64 target of course isn't very useful - most cores implement at least some of the standard extensions.

I'd also be very wary of users depending on the default ABI selection.

So I agree with @jrtc27 here - if any LLVM IR producing frontends aren't explicitly setting the ABI or providing a way to do so, they really need to. You need to be aware of the target ABI when generating IR of course - many things will appear fine if you don't, but you'll run in to corner cases. See clang/lib/CodeGen/TargetInfo.cpp for how Clang handles this. It's come up many times in the LLVM community that it would be nice to avoid the need for this, but nobody has taken it on so far.

I'd also be very wary of users depending on the default ABI selection.

Fair enough, but you also said that you had no real objection to changing the default. In practice this would really save us a ton of trouble (because of Windows and frustrating issues with package managers). So, could you change the default anyway?

Also note that the LLVM C API, which many libraries use including inkwell/llvm-sys, does not expose the ABI settings. Getting the default right most of the time would, in practice, help with using LLVM.

If that’s the case then it needs to be extended. Changing the default is not a fix, it’s a bodge.

Nor do I see how Windows or package managers are relevant to a discussion about how software should use LLVM’s APIs for compiling RISC-V code.

Because modifying a stack of libraries is a pain when dealing with conda/cargo/Windows. Try it if you don't believe me.

jrtc27 added a comment.Dec 9 2021, 7:46 AM

FYI the llvmlite PR to set MCOptions.ABIName got merged.

I continue to believe that this should not be necessary from an LLVM API consumer perspective. Anything not setting the ABI is broken and should be fixed, and it's not our problem that they don't use our API properly or exist in an ecosystem that makes it difficult to patch them. If anything I would rather there be no default in the backend, only in the Clang frontend, but the main reason not to do that is it's annoying for writing tests where most don't care what floating-point ABI is in use.

Whatever. I figured out how to cross-compile LLVM for Windows with mingw-gcc, so you can continue your bike-shedding, this is not my problem anymore as I can ship my own build. And, going forward, I will not waste my time sending patches to LLVM since the lack of pragmatism here is just ridiculous. This patch does not degrade the code base in any way.