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

[Sam Elliott via Phabricator via llvm-commits]

lenary added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

This is looking good.

I remember we discussed this on the LLVM call a few weeks ago - there was a discussion as to whether we should be prioritising `-march` or `-mcpu` - do you recall the outcome of that discussion?



================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:166
+bool RISCV32TargetInfo::isValidCPUName(StringRef Name) const {
+  return llvm::RISCV::checkCPUKind(llvm::RISCV::parseCPUKind(Name), false);
+}
----------------
It would be good to have `/*Is64Bit=*/` before the boolean arguments to these functions.


================
Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:501-516
-  // 3. Choose a default based on the triple
-  //
-  // We deviate from GCC's defaults here:
-  // - On `riscv{XLEN}-unknown-elf` we use the integer calling convention only.
-  // - On all other OSs we use the double floating point calling convention.
-  if (Triple.getArch() == llvm::Triple::riscv32) {
-    if (Triple.getOS() == llvm::Triple::UnknownOS)
----------------
What's the justification for removing this code?


================
Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:310
+  case llvm::Triple::riscv64:
+    if (const Arg *A = Args.getLastArg(options::OPT_mcpu_EQ))
+      return A->getValue();
----------------
Should we be doing more validation here?


================
Comment at: llvm/lib/Support/TargetParser.cpp:122
 
+struct CPUInfo {
+  StringLiteral Name;
----------------
I think this should be prefixed `RISCV`, or moved into the RISCV namespace if you can?


================
Comment at: llvm/lib/Support/TargetParser.cpp:259-260
+
+  if (features & FK_64BIT)
+    Features.push_back("+64bit");
+
----------------
It might be worth explicitly adding `-64bit` where FK_64BIT is not set.


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

https://reviews.llvm.org/D71124