Page MenuHomePhabricator

[RISCV] Match GCC `-march`/`-mabi` driver defaults
ClosedPublic

Authored by lenary on Oct 24 2019, 6:54 AM.

Details

Summary

Clang/LLVM is a cross-compiler, and so we don't have to make a choice
about -march/-mabi at build-time, but we may have to compute a
default -march/-mabi when compiling a program. Until now, each
place that has needed a default -march has calculated one itself.

This patch adds a single place where a default -march is calculated,
in order to avoid calculating different defaults in different places.

This patch adds a new function riscv::getRISCVArch which encapsulates
this logic based on GCC's for computing a default -march value
when none is provided. This patch also updates the logic in
riscv::getRISCVABI to match the logic in GCC's build system for
computing a default -mabi.

This patch also updates anywhere that -march is used to now use the
new function which can compute a default. In particular, we now
explicitly pass a -march value down to the gnu assembler.

GCC has convoluted logic in its build system to choose a default
-march/-mabi based on build options, which would be good to match.
This patch is based on the logic in GCC 9.2.0. This commit's logic is
different to GCC's only for baremetal targets, where we default
to rv32imac/ilp32 or rv64imac/lp64 depending on the target triple.

Tests have been updated to match the new logic.

Diff Detail

Event Timeline

lenary created this revision.Oct 24 2019, 6:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2019, 6:54 AM
lenary updated this revision to Diff 226252.Oct 24 2019, 6:57 AM
  • Correct code formatting issue
Harbormaster completed remote builds in B39998: Diff 226252.
rogfer01 added inline comments.Oct 24 2019, 9:48 AM
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
476

llvm::StringSwitch has a method StartsWithLower which might help make the logic a bit clearer

Something like this (I haven't tested it!)

StringRef ABIFromMarch = StringSwitch(MArch)
   .StartsWithLower("rv32d", "ilp32d")
   .StartsWithLower("rv32g", "ilp32d")
   .StartsWithLower("rv32e", "ilp32e")
   .StartsWithLower("rv32", "ilp32")

   .StartsWithLower("rv64d", "lp64d")
   .StartsWithLower("rv64g", "lp64d")
   .StartsWithLower("rv64", "lp64").

   .Default("");

if (!ABIFromMarch.empty()) return ABIFromMarch;
lenary marked an inline comment as done.Oct 24 2019, 10:09 AM
lenary added inline comments.
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
476

Sadly I don't think this will work, because of the case of matching rv32*d* and rv64*d* (the March.substr(4).contains_lower("d") cases) from config.gcc. Explicitly "d" does not come immediately after rv<32/64>, it can come anywhere after like in rv32imafdc.

The other issue I have with the StringSwitch is that it requires I have a default, which I feel makes the control flow harder to understand, rather than easier.

khchen added inline comments.Oct 24 2019, 11:12 AM
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
538

Why do you set rv32gc and rv64gc as default march? Is there a any reason?

lenary marked an inline comment as done.Oct 24 2019, 11:15 AM
lenary added inline comments.
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
538

This reflects the logic in [[ https://github.com/riscv/riscv-gcc/blob/riscv-gcc-9.2.0/gcc/config.gcc#L4229-L4369 | config.gcc ]]. Note line 4273, which applies if neither arch nor abi is given (the xlen is set on line 4233-4234 based on the target triple).

rogfer01 added inline comments.Oct 24 2019, 1:21 PM
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
476

Oh I see.

Then I would comment what this part does with a bit more detail right after the // 2. Choose a default based on -march=. For example

// rv32g | rv32*d -> ilp32d
// rv32e -> ilp32e
// rv32* -> ilp32
// rv64g | rv64*d -> lp64d
// rv64* -> lp64

Given that gcc is using a shell glob, then GlobPattern in Support/GlobPattern.h may help as well. But it might be overkill in this scenario.

I have a question about backwards compatibility with this patch. Clang 9 has shipped with rvXXg/etc defaulting to ilp32/lp64 ABI, and no march meaning rvXXi, with users having built objects with those defaults. When Clang 10 ships, users they now need to always use a mabi/march flag to keep the same compatibility with their Clang 9 flows, the enabled extensions and ABI will be changed under their feet without warning (or at least until they either hit a linker error in the ABI case, or potentially an invalid instruction trap in the march case).

If we're going to change the defaults, this patch should at least contain an update to the release notes file, this way this change would be documented for users.

clang/lib/Driver/ToolChains/Arch/RISCV.cpp
477

Won’t this break if the user specifies a X/Z extension that has a d in the name, so eg rv32iXd will try to use the ilp32d abi by default? For future proofing, I think we may need to do a full parse of the isa string to verify that d does actually mean the standard D-extension

lenary marked an inline comment as done.Oct 25 2019, 3:21 AM

I agree backwards compatibility is hard here.

  • This method was introduced to have a single place to choose a default march string if none was chosen before. I think this change is useful (saves defaults being calculated in a multitude of other places, which means they may not agree).
  • The choice of a default march string was based, as closely as possible, on config.gcc. This may not be what we want (and is the source of backwards compatibility issues).
  • I 100% agree that a release notes entry is needed. I will write one just as soon as we finalize the default behaviour.
  • The elf multilib changes in D67508 seem to be more complex, as config.gcc chooses a march/mabi default that is not built by t-elf-multilib.

I think I would understand updating the logic to use fewer architecture extensions for a given ABI (i.e., default to rv32i unless someone asks for ilp32f/d) rather than more (rv32gc when someone asks for ilp32). This should be more backwards compatible, but also requires carefully ensuring the logic is not circular.

At the very least, I do not plan to merge this patch until we have had a chance to discuss it next Thursday (31st Oct) on the RISC-V backend call.

clang/lib/Driver/ToolChains/Arch/RISCV.cpp
477

You're right that having "d" in any extension name will cause issues. The same is true for any gcc build today (9.2.0).

lenary updated this revision to Diff 227284.Oct 31 2019, 8:19 AM
  • More conservative march/mabi defaults on riscv-unknown-elf
  • Add clang release notes about this change to -march/-mabi
lenary updated this revision to Diff 227418.Nov 1 2019, 4:41 AM
  • Update tests to reflect new defaults
khchen added a comment.Nov 2 2019, 8:33 AM

LGTM, thanks for the patch!

lenary edited the summary of this revision. (Show Details)Nov 11 2019, 10:06 AM
lenary updated this revision to Diff 228728.Nov 11 2019, 10:12 AM
  • Rebase
  • Update comments

I think this is just about ready to land, given our discussion on the RISC-V call on 7 November 2019.

This revision is now accepted and ready to land.Nov 14 2019, 7:34 AM
asb accepted this revision.Nov 14 2019, 7:41 AM

Please update the commit message to clarify the cases where we do deviate from the GCC defaults, but this looks good to me.

lenary edited the summary of this revision. (Show Details)Nov 15 2019, 7:08 AM
lenary updated this revision to Diff 229542.Nov 15 2019, 7:09 AM

Rebase and update comments and release notes

This revision was automatically updated to reflect the committed changes.