[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

178

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

252

The ISAInfo includes XLen

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

200

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

llvm/lib/Support/TargetParser.cpp

336

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

250

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

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

249

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.cpp

handleAllErrors(ParseResult.takeError(), [&](llvm::StringError &ErrMsg) {
       OutputErrMsg << ErrMsg.getMessage();
     });
     Diags.Report(diag::err_invalid_feature_combination) << OutputErrMsg.str();
     return false;
   } else {
     ISAInfo = std::move(*ParseResult);
   }
 
+  if (ABI.empty())
+    ABI = llvm::RISCV::computeDefaultABIFromArch(*ISAInfo).str();
+
   return true;
 }
 
 bool RISCV32TargetInfo::isValidCPUName(StringRef Name) const {
   return llvm::RISCV::checkCPUKind(llvm::RISCV::parseCPUKind(Name),
                                    /*Is64Bit=*/false);
 }
 
 void RISCV32TargetInfo::fillValidCPUList(

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

StringRef riscv::getRISCVABI(const ArgList &Args, const llvm::Triple &Triple) {
   // rv32e -> ilp32e
   // rv32* -> ilp32
   // rv64g | rv64*d -> lp64d
   // rv64* -> lp64
   StringRef Arch = getRISCVArch(Args, Triple);
 
   auto ParseResult = llvm::RISCVISAInfo::parseArchString(
       Arch, /* EnableExperimentalExtension */ true);
-  if (!ParseResult) {
+  if (!ParseResult)
     // Ignore parsing error, just go 3rd step.
     consumeError(ParseResult.takeError());
-  } else {
-    auto &ISAInfo = *ParseResult;
-    bool HasD = ISAInfo->hasExtension("d");
-    unsigned XLen = ISAInfo->getXLen();
-    if (XLen == 32) {
-      bool HasE = ISAInfo->hasExtension("e");
-      if (HasD)
-        return "ilp32d";
-      if (HasE)
-        return "ilp32e";
-      return "ilp32";
-    } else if (XLen == 64) {
-      if (HasD)
-        return "lp64d";
-      return "lp64";
-    }
-    llvm_unreachable("unhandled XLen");
-  }
+  else
+    return llvm::RISCV::computeDefaultABIFromArch(**ParseResult);
 
   // 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)

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
 
+// Test expected behavior when giving -target-cpu
+// This cc1 test is similar to clang with -march=rv32ifd -mcpu=sifive-e31, default abi is ilp32d
+// RUN: %clang_cc1 -triple riscv32 -emit-llvm -target-feature +f -target-feature +d -target-cpu sifive-e31 -o - %s | FileCheck -check-prefix=EMPTY-ILP32D %s
+// This cc1 test is similar to clang with -march=rv64i -mcpu=sifive-u74, default abi is lp64
+// RUN: %clang_cc1 -triple riscv64 -emit-llvm -o - -target-cpu sifive-u74 %s | FileCheck -check-prefix=EMPTY-LP64 %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"}

llvm/include/llvm/Support/TargetParser.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_TARGETPARSER_H
 #define LLVM_SUPPORT_TARGETPARSER_H
 
 // FIXME: vector is used because that's what clang uses for subtarget feature
 // lists, but SmallVector would probably be better
 #include "llvm/ADT/Triple.h"
-#include "llvm/Support/ARMTargetParser.h"
 #include "llvm/Support/AArch64TargetParser.h"
+#include "llvm/Support/ARMTargetParser.h"
+#include "llvm/Support/RISCVISAInfo.h"
 #include <vector>
 
 namespace llvm {
 class StringRef;
 
 // Target specific information in their own namespaces.
 // (ARM/AArch64/X86 are declared in ARM/AArch64/X86TargetParser.h)
 // These should be generated from TableGen because the information is already

 bool checkTuneCPUKind(CPUKind Kind, bool IsRV64);
 CPUKind parseCPUKind(StringRef CPU);
 CPUKind parseTuneCPUKind(StringRef CPU, bool IsRV64);
 StringRef getMArchFromMcpu(StringRef CPU);
 void fillValidCPUArchList(SmallVectorImpl<StringRef> &Values, bool IsRV64);
 void fillValidTuneCPUArchList(SmallVectorImpl<StringRef> &Values, bool IsRV64);
 bool getCPUFeaturesExceptStdExt(CPUKind Kind, std::vector<StringRef> &Features);
 StringRef resolveTuneCPUAlias(StringRef TuneCPU, bool IsRV64);
+StringRef computeDefaultABIFromArch(const llvm::RISCVISAInfo &ISAInfo);
 
 } // namespace RISCV
 
 } // namespace llvm
 
 #endif

llvm/lib/Support/TargetParser.cpp

bool getCPUFeaturesExceptStdExt(CPUKind Kind,
   if (CPUFeatures & FK_64BIT)
     Features.push_back("+64bit");
   else
     Features.push_back("-64bit");
 
   return true;
 }
 
+StringRef computeDefaultABIFromArch(const llvm::RISCVISAInfo &ISAInfo) {
+  if (ISAInfo.getXLen() == 32) {
+    if (ISAInfo.hasExtension("d"))
+      return "ilp32d";
+    if (ISAInfo.hasExtension("e"))
+      return "ilp32e";
+    return "ilp32";
+  } else if (ISAInfo.getXLen() == 64) {
+    if (ISAInfo.hasExtension("d"))
+      return "lp64d";
+    return "lp64";
+  }
+  llvm_unreachable("Invalid XLEN");
+}
+
 } // namespace RISCV
 } // namespace llvm