[PATCH] D71124: [RISCV] support clang driver to select cpu

Wed Jul 8 21:52:20 PDT 2020

asb added a comment.

This has been hanging around for a while, but I think we'd basically agreed this is the right logic. The comments have ended up referring to flags that don't exist on Clang making it a little hard to follow, and I've added a request to slightly expand testing. If you make those cleanups I think it should be ready for a final review and merge.

As Sam says, lets flag this in today's RISC-V LLVM call to double-check everyone is happy.

================
Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:622
+  // 1. Explicit choices using `--with-arch=`
+  // 2. Based on `-mcpu` if target cpu has default isa extension feature
+  // 3. A default based on `--with-abi=`, if provided
----------------
As clang has no with-arch or with-abi, this comment seems inaccurate?

================
Comment at: clang/test/Driver/riscv-cpus.c:2
+// Check target CPUs are correctly passed.
+
+// RUN: %clang -target riscv32 -### -c %s 2>&1 -mcpu=rocket-rv32 | FileCheck -check-prefix=MCPU-ROCKETCHIP32 %s
----------------
I think for completeness this test should be validating the interaction of the ABI choosing logic with CPU selection as well. With the implemented logic I believe it should show that lp64d is selected for -mcpu=sifive-u54 and that -mcpu=sifive-u54 -mabi=lp64 will respect the ABI choice

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71124