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

Fri Oct 25 01:34:06 PDT 2019

simoncook added a comment.

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.

================
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"))
----------------
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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69383