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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
