[RISCV][Clang] Compute the default target-abi if it's empty.

[khchen]

Summary

Every generated IR has corresponding target-abi value, so encode a non-empty value would improve the robustness and correctness than empty one.

[jrtc27]

Extract the code out into a shared function, don't duplicate it otherwise we'll get confusing inconsistent defaults. Especially when there are proposals to change the defaults as currently there are some weird interactions where specifying seemingly-redundant flags changes the behaviour (e.g. clang -target riscv32-unknown-elf gets you rv32imac/ilp32, but clang -target riscv32-unknown-elf -mabi=ilp32 gets you rv32imafdc/ilp32).

[khchen]

address jrtc27's comment.

[khchen]

rebase on D105168.

[asb]

This looks like a good improvement to me - anything that still makes it a "[PoC]" proof of concept?

[arichardson]

llvm/include/llvm/Support/TargetParser.h

177 ↗

(On Diff #359670)

Maybe this should be computeDefaultABIFromArch()? If llvm::RISCVISAInfo can be used here passing const llvm::RISCVISAInfo& to the function would also avoid some duplicated lines.

[khchen]

address arichardson's comment, thanks!

[khchen]

[jrtc27]

clang/lib/Basic/Targets/RISCV.cpp

335

The ISAInfo includes XLen

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

202 ↗

(On Diff #361216)

Ditto (it's literally on the line above!)

llvm/lib/Support/TargetParser.cpp

336 ↗

(On Diff #361216)

I'm not personally a fan of having HasD but using ISAInfo.hasExtension("e") in the if. I get why it's done, but I think consistency would be better. Personally I'd just inline the ISAInfo.hasExtension("d"), it's not worth a local variable.

[jrtc27]

clang/lib/Basic/Targets/RISCV.cpp

333

I don't think using the getter makes sense. Currently it's entirely equivalent to just reading ABI so serves little purpose, but in theory there could be additional logic added to it (like asserting it's not empty) that would break this. Given you already assume the name of the field below by assigning to it, just read ABI directly.

[khchen]

address @jrtc27's comment, thanks! I forget RISCVISAInfo include XLen..

[jrtc27]

llvm/lib/Support/TargetParser.cpp

341–344 ↗

(On Diff #361264)

makes me feel more comfortable inside, and also is a bit more like the old code where we did explicitly check 32 and 64 (though that instead fell back on the next step, rather than asserting).

[jrtc27]

The commit message needs rewriting to reflect the final patch. Also, "Explicitly target-abi encoded in IR is clear than empty target-abi." is wrong, it's not about clarity, it's about robustness and correctness.

clang/lib/Basic/Targets/RISCV.cpp

332

I don't think this warrants a comment, and the current grammar is a bit dodgy, but if you still want one: "Compute the default ABI based on the target features"

[khchen]

address @jrtc27's comment, thanks again!

[asb]

This looks good to me, and as Jessica says this patch improves robustness and correctness so it would be great to land it for 13.x.

@jrtc27 - I think @khchen has reflected all of your comments, but please double-check if you get a chance.

[jrtc27]

Commit message needs rewriting still, but the patch looks good to me

[khchen]

This patch depends on D105168,
Please help to review it if you have time.
Thanks!

[khchen]

rebase

clang/lib/Basic/Targets/RISCV.h

public:
 
   bool setCPU(const std::string &Name) override {
     if (!isValidCPUName(Name))
       return false;
     CPU = Name;
     return true;
   }
 
-  StringRef getABI() const override { return ABI; }
   void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override;
 
   ArrayRef<Builtin::Info> getTargetBuiltins() const override;
 
   BuiltinVaListKind getBuiltinVaListKind() const override {
     return TargetInfo::VoidPtrBuiltinVaList;
   }
public:
   bool setABI(const std::string &Name) override {
     if (Name == "ilp32" || Name == "ilp32f" || Name == "ilp32d") {
       ABI = Name;
       return true;
     }
     return false;
   }
 
+  StringRef getABI() const override;
+
   bool isValidCPUName(StringRef Name) const override;
   void fillValidCPUList(SmallVectorImpl<StringRef> &Values) const override;
   bool isValidTuneCPUName(StringRef Name) const override;
   void fillValidTuneCPUList(SmallVectorImpl<StringRef> &Values) const override;
 
   void setMaxAtomicWidth() override {
     MaxAtomicPromoteWidth = 128;
 
public:
   bool setABI(const std::string &Name) override {
     if (Name == "lp64" || Name == "lp64f" || Name == "lp64d") {
       ABI = Name;
       return true;
     }
     return false;
   }
 
+  StringRef getABI() const override;
+
   bool isValidCPUName(StringRef Name) const override;
   void fillValidCPUList(SmallVectorImpl<StringRef> &Values) const override;
   bool isValidTuneCPUName(StringRef Name) const override;
   void fillValidTuneCPUList(SmallVectorImpl<StringRef> &Values) const override;
 
   void setMaxAtomicWidth() override {
     MaxAtomicPromoteWidth = 128;
 
     if (HasA)
       MaxAtomicInlineWidth = 64;
   }
 };
 } // namespace targets
 } // namespace clang
 
 #endif // LLVM_CLANG_LIB_BASIC_TARGETS_RISCV_H

clang/lib/Basic/Targets/RISCV.cpp

for (const auto &Feature : Features) {
     else if (Feature == "+experimental-zfh")
       HasZfh = true;
     else if (Feature == "+experimental-zvamo")
       HasZvamo = true;
     else if (Feature == "+experimental-zvlsseg")
       HasZvlsseg = true;
   }
 
   return true;
 }
 
+StringRef RISCV32TargetInfo::getABI() const {
+  if (!ABI.empty())
+    return ABI;
+  // Choose a default based on the target architecture.
+  // The logic sync with riscv::getRISCVABI
+  if (HasD)
+    return "ilp32d";
+  return "ilp32";
+}
+
 bool RISCV32TargetInfo::isValidCPUName(StringRef Name) const {
   return llvm::RISCV::checkCPUKind(llvm::RISCV::parseCPUKind(Name),
                                    /*Is64Bit=*/false);
 }
 
 void RISCV32TargetInfo::fillValidCPUList(
     SmallVectorImpl<StringRef> &Values) const {
   llvm::RISCV::fillValidCPUArchList(Values, false);
 }
 
 bool RISCV32TargetInfo::isValidTuneCPUName(StringRef Name) const {
   return llvm::RISCV::checkTuneCPUKind(
       llvm::RISCV::parseTuneCPUKind(Name, false),
       /*Is64Bit=*/false);
 }
 
 void RISCV32TargetInfo::fillValidTuneCPUList(
     SmallVectorImpl<StringRef> &Values) const {
   llvm::RISCV::fillValidTuneCPUArchList(Values, false);
 }
 
+StringRef RISCV64TargetInfo::getABI() const {
+  if (!ABI.empty())
+    return ABI;
+  // Choose a default based on the target architecture.
+  // The logic sync with riscv::getRISCVABI
+  if (HasD)
+    return "lp64d";
+  return "lp64";
+}
+
 bool RISCV64TargetInfo::isValidCPUName(StringRef Name) const {
   return llvm::RISCV::checkCPUKind(llvm::RISCV::parseCPUKind(Name),
                                    /*Is64Bit=*/true);
 }
 
 void RISCV64TargetInfo::fillValidCPUList(
     SmallVectorImpl<StringRef> &Values) const {
   llvm::RISCV::fillValidCPUArchList(Values, true);

clang/test/CodeGen/RISCV/riscv-metadata.c

+// RUN: %clang_cc1 -triple riscv32 -emit-llvm -o - %s | FileCheck -check-prefix=EMPTY-ILP32 %s
+// RUN: %clang_cc1 -triple riscv32 -emit-llvm -target-feature +f -target-feature +d -o - %s | FileCheck -check-prefix=EMPTY-ILP32D %s
 // RUN: %clang_cc1 -triple riscv32 -target-abi ilp32 -emit-llvm -o - %s | FileCheck -check-prefix=ILP32 %s
 // RUN: %clang_cc1 -triple riscv32 -target-feature +f -target-abi ilp32f -emit-llvm -o - %s | FileCheck -check-prefix=ILP32F %s
 // RUN: %clang_cc1 -triple riscv32 -target-feature +d -target-abi ilp32d -emit-llvm -o - %s | FileCheck -check-prefix=ILP32D %s
+// RUN: %clang_cc1 -triple riscv64 -emit-llvm -o - %s | FileCheck -check-prefix=EMPTY-LP64 %s
+// RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-feature +d -emit-llvm -o - %s | FileCheck -check-prefix=EMPTY-LP64D %s
 // RUN: %clang_cc1 -triple riscv64 -target-abi lp64 -emit-llvm -o - %s | FileCheck -check-prefix=LP64 %s
 // RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-abi lp64f -emit-llvm -o - %s | FileCheck -check-prefix=LP64F %s
 // RUN: %clang_cc1 -triple riscv64 -target-feature +d -target-abi lp64d -emit-llvm -o - %s | FileCheck -check-prefix=LP64D %s
 
+// EMPTY-ILP32: !{{[0-9]+}} = !{i32 1, !"target-abi", !"ilp32"}
+// EMPTY-ILP32D: !{{[0-9]+}} = !{i32 1, !"target-abi", !"ilp32d"}
 // ILP32: !{{[0-9]+}} = !{i32 1, !"target-abi", !"ilp32"}
 // ILP32F: !{{[0-9]+}} = !{i32 1, !"target-abi", !"ilp32f"}
 // ILP32D: !{{[0-9]+}} = !{i32 1, !"target-abi", !"ilp32d"}
 
+// EMPTY-LP64: !{{[0-9]+}} = !{i32 1, !"target-abi", !"lp64"}
+// EMPTY-LP64D: !{{[0-9]+}} = !{i32 1, !"target-abi", !"lp64d"}
 // LP64: !{{[0-9]+}} = !{i32 1, !"target-abi", !"lp64"}
 // LP64F: !{{[0-9]+}} = !{i32 1, !"target-abi", !"lp64f"}
 // LP64D: !{{[0-9]+}} = !{i32 1, !"target-abi", !"lp64d"}