[PATCH] D69383: [RISCV] Match GCC `-march`/`-mabi` driver defaults

[Sam Elliott via Phabricator via cfe-commits]

lenary marked an inline comment as done.
lenary added a comment.

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 <https://reviews.llvm.org/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.



================
Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:476
+    if (MArch.startswith_lower("rv32")) {
+      if (MArch.substr(4).contains_lower("d") ||
+          MArch.startswith_lower("rv32g"))
----------------
simoncook wrote:
> 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
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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69383/new/

https://reviews.llvm.org/D69383