[RISCV] Report an error when ABI mismatch with target-abi module flag.

[khchen]

Summary

D72768 allows users to give an empty target-abi option with non-empty
target-abi module flag in IR, because we attempts to allowing module flag
to overwrite the empty target-abi options (D72624).

ps. It's worth mentioning that datalayout tends to change for different ABIs.

After digging more into LLVM TargetMahcine design, and as @efriedma mention in
https://reviews.llvm.org/D71387#1788472, overwrite the ABI/datalayout in
TargetMachine or a bitcode file is not easy and it will generate wrong code.
(For example, IR parser supports a datalayout callback which's used to compute
a correct datalayout during the IR parsing, see D78403 for more detal. So if
we want to read the ABI/datalayout info from IR first, it seems we need to
parse the IR twice times, it does not make sense for me.)

In this situation, I think probably reporting a error when the default ABI from empty target-abi option
mismatch with target-abi module flag.
This changed requires users need to specific -target-abi option if IR has
-target-abi module flag if default is mismatch.
In other words, for enabling LTO in RISC-V, clang driver must to forward the
target-abi option into LTO code generator.

ps. The checking routine only exists in the RISC-V back-end now because only
RISC-V will generate -target-abi module flag.

Reference: https://lists.llvm.org/pipermail/llvm-dev/2020-January/138450.html

[jrtc27]

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp

95–115

Please spell-check this and correctly capitalise things like DataLayout.

[jrtc27]

llvm/test/CodeGen/RISCV/module-target-abi.ll

6–7

These error messages should be much more similar as they're basically the same. And as part of that, include the two ABIs in the mismatch error message, not just that there was a mismatch.

[khchen]

1. address @jrtc27' comment
2. change report_fatal_error as errs() because report_fatal_error will ask users to submit a bug report, it does not make sense.
3. combine two error message as one.

[khchen]

addres https://reviews.llvm.org/D71387#2762120, compute a default ABI for comparison rather than report an error when missing -mabi

[craig.topper]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

293

Is ABIName still the right name here since it isn't a string?

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp

108

This just prints to std::error, but doesn't stop the build. That makes it easy for the user to ignore. Is that what we want?

llvm/test/CodeGen/RISCV/module-target-abi2.ll

6

Currnet->Current

[khchen]

address Craig's comments, thanks!

[khchen]

ping

[khchen]

1. Handle an empty module flag.

So sorry I forgot to run the clang regresion tests before.

2. Add empty module flag test and combine all tests into one. It is simiar to compress.ll

[luismarques]

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp

115

"Mismatch" ABI -> "Mismatched ABIs" or "Mismatching ABIs".

llvm/test/CodeGen/RISCV/module-target-abi-tests.ll

5 ↗

(On Diff #347356)

Is this something that we are handling in general, having such flag without a value?

[khchen]

address @luismarques's comments.

llvm/test/CodeGen/RISCV/module-target-abi-tests.ll

5 ↗

(On Diff #347356)

Good cached, in general the target-abi is not empty, I updated the current implementation, thanks!

[khchen]

llvm/test/CodeGen/RISCV/module-target-abi-tests.ll

5 ↗

(On Diff #347356)

@luismarques

Sorry, I forget that the empty target-abi are coming from some clang cc1 tests.
They are missing -target-abi option in clang cc1 so target-abi module flag is empty.

CodeGen/RISCV/riscv-atomics.c
CodeGen/RISCV/riscv-inline-asm-rvv.c
CodeGen/RISCV/riscv-inline-asm-xsfvfhbfmin.c
CodeGen/RISCV/riscv-inline-asm.c

Maybe we need to calculate the default target-abi if it's empty? or handle empty target-abi in the backend?

[khchen]

Revert to previous revision Diff 347356 and add empty module flag could be empty in test.

[khchen]

ping

[jrtc27]

llvm/test/CodeGen/RISCV/module-target-abi-tests.ll

5 ↗

(On Diff #347356)

I do think we should be filling in the default ABI here, otherwise it's very fragile.

[khchen]

llvm/test/CodeGen/RISCV/module-target-abi-tests.ll

5 ↗

(On Diff #347356)

Do you mean cc1 need to calculate the default target-abi and fill it in IR?

[jrtc27]

llvm/test/CodeGen/RISCV/module-target-abi-tests.ll

5 ↗

(On Diff #347356)

Yes. Otherwise if Clang and LLVM ever disagree on the default ABI for any particular case then you will get inconsistent code, as the C source-visible ABI will use one but the code generation will use another. It also means you can never change the default ABI in LLVM since otherwise you will break existing bitcode files that rely on the default.

[khchen]

rebase on D102582
report a error if target-abi module flag is is empty.

[khchen]

we don't need this patch because we already handle the empty target-abi module flag now.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

 };
 
 // Returns the target ABI, or else a StringError if the requested ABIName is
 // not supported for the given TT and FeatureBits combination.
 ABI computeTargetABI(const Triple &TT, FeatureBitset FeatureBits,
                      StringRef ABIName);
 
 ABI getTargetABI(StringRef ABIName);
+StringRef getTargetABI(ABI ABIName);
 
 // Returns the register used to hold the stack pointer after realignment.
 MCRegister getBPReg();
 
 // Returns the register holding shadow call stack pointer.
 MCRegister getSCSPReg();
 
 } // namespace RISCVABI

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp

auto TargetABI = StringSwitch<ABI>(ABIName)
                        .Case("ilp32e", ABI_ILP32E)
                        .Case("lp64", ABI_LP64)
                        .Case("lp64f", ABI_LP64F)
                        .Case("lp64d", ABI_LP64D)
                        .Default(ABI_Unknown);
   return TargetABI;
 }
 
+StringRef getTargetABI(ABI ABIName) {
+  switch (ABIName) {
+  default:
+    llvm_unreachable("Unknown ABI!");
+  case ABI_ILP32:
+    return "ilp32";
+  case ABI_ILP32F:
+    return "ilp32f";
+  case ABI_ILP32D:
+    return "ilp32d";
+  case ABI_ILP32E:
+    return "ilp32e";
+  case ABI_LP64:
+    return "lp64";
+  case ABI_LP64F:
+    return "lp64f";
+  case ABI_LP64D:
+    return "lp64d";
+  }
+}
+
 // To avoid the BP value clobbered by a function call, we need to choose a
 // callee saved register to save the value. RV32E only has X8 and X9 as callee
 // saved registers and X8 will be used as fp. So we choose X9 as bp.
 MCRegister getBPReg() { return RISCV::X9; }
 
 // Returns the register holding shadow call stack pointer.
 MCRegister getSCSPReg() { return RISCV::X18; }
 

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp

std::string FS =
       FSAttr.isValid() ? FSAttr.getValueAsString().str() : TargetFS;
   std::string Key = CPU + TuneCPU + FS;
   auto &I = SubtargetMap[Key];
   if (!I) {
     // This needs to be done before we create a new subtarget since any
     // creation will depend on the TM and the code generation flags on the
     // function that reside in TargetOptions.
     resetTargetOptions(F);
-    auto ABIName = Options.MCOptions.getABIName();
-    if (const MDString *ModuleTargetABI = dyn_cast_or_null<MDString>(
+
+    I = std::make_unique<RISCVSubtarget>(TargetTriple, CPU, TuneCPU, FS,
+                                         Options.MCOptions.getABIName(), *this);
+    // The ABI should equal target-abi module flag because the different ABI
+    // has its corresponding datalayout and IR alignment information. In
+    // addition, the IR parser need to have datalayout information before it
+    // gets the ABI info from module. It does not make sense to update the
+    // module's datalayout and alignment information after IR parsing.
+    if (const MDString *MDModuleABI = dyn_cast_or_null<MDString>(
             F.getParent()->getModuleFlag("target-abi"))) {
-      auto TargetABI = RISCVABI::getTargetABI(ABIName);
-      if (TargetABI != RISCVABI::ABI_Unknown &&
-          ModuleTargetABI->getString() != ABIName) {
-        report_fatal_error("-target-abi option != target-abi module flag");
-      }
-      ABIName = ModuleTargetABI->getString();
+      StringRef ModuleABIName = MDModuleABI->getString();
+      // Get the default ABI when target-abi option is missing.
+      RISCVABI::ABI TargetABI = I->getTargetABI();
+      if (TargetABI != RISCVABI::getTargetABI(ModuleABIName))
+        errs() << "Mismatch ABI. Currnet ABI is '" +
+                      RISCVABI::getTargetABI(TargetABI) +
+                      "', but IR target-abi module flag is '" + ModuleABIName +
+                      "'.\n";
     }
-    I = std::make_unique<RISCVSubtarget>(TargetTriple, CPU, TuneCPU, FS, ABIName, *this);
   }
   return I.get();
 }
 
 TargetTransformInfo
 RISCVTargetMachine::getTargetTransformInfo(const Function &F) {
   return TargetTransformInfo(RISCVTTIImpl(this, F));
 }
 
 // A RISC-V hart has a single byte-addressable address space of 2^XLEN bytes
 // for all memory accesses, so it is reasonable to assume that an

llvm/test/CodeGen/RISCV/module-target-abi.ll

-; RUN: llc -mtriple=riscv32 < %s 2>&1 \
-; RUN:   | FileCheck -check-prefix=DEFAULT %s
-; RUN: llc -mtriple=riscv32 -target-abi ilp32 < %s 2>&1 \
-; RUN:   | FileCheck -check-prefix=RV32IF-ILP32 %s
-; RUN: not --crash llc -mtriple=riscv32 -target-abi ilp32f < %s 2>&1 \
-; RUN:   | FileCheck -check-prefix=RV32IF-ILP32F %s
-; RUN: llc -mtriple=riscv32 -filetype=obj < %s | llvm-readelf -h - | FileCheck -check-prefixes=FLAGS %s
+; Default rv32 ABI is ilp32 which equals target-abi module flag, so the result is correct.
+; RUN: llc -mtriple=riscv32 < %s -o /dev/null
+; RUN: llc -mtriple=riscv32 -target-abi ilp32f < %s -o /dev/null 2>&1 \
+; RUN:   | FileCheck %s
 
-; RV32IF-ILP32F: -target-abi option != target-abi module flag
-
-; FLAGS: Flags: 0x0
+; CHECK: Mismatch ABI. Currnet ABI is 'ilp32f', but IR target-abi module flag is 'ilp32'.
 
 define float @foo(i32 %a) nounwind #0 {
-; DEFAULT: # %bb.0:
-; DEFAULT: fmv.x.w a0, ft0
-; RV32IF-ILP32: # %bb.0:
-; RV32IF-ILP32: fmv.x.w a0, ft0
   %conv = sitofp i32 %a to float
   ret float %conv
 }
 
 attributes #0 = { "target-features"="+f"}
 !llvm.module.flags = !{!0}
 !0 = !{i32 1, !"target-abi", !"ilp32"}

llvm/test/CodeGen/RISCV/module-target-abi2.ll

-; RUN: llc -mtriple=riscv32 < %s 2>&1 \
-; RUN:   | FileCheck -check-prefix=DEFAULT %s
-; RUN: not --crash llc -mtriple=riscv32 -target-abi ilp32 < %s 2>&1 \
-; RUN:   | FileCheck -check-prefix=RV32IF-ILP32 %s
-; RUN: llc -mtriple=riscv32 -target-abi ilp32f < %s 2>&1 \
-; RUN:   | FileCheck -check-prefix=RV32IF-ILP32F %s
-; RUN: llc -mtriple=riscv32 -filetype=obj < %s | llvm-readelf -h - | FileCheck -check-prefixes=FLAGS %s
+; Default rv32 ABI is ilp32 which mismatch with target-abi module flag ilp32f
+; RUN: llc -mtriple=riscv32 < %s -o /dev/null 2>&1 | FileCheck %s
+; RUN: llc -mtriple=riscv32 -target-abi ilp32 < %s -o /dev/null 2>&1 \
+; RUN:   | FileCheck %s
 
-; RV32IF-ILP32: -target-abi option != target-abi module flag
-
-; FLAGS: Flags: 0x0
-; // this should be "Flags :0x2, single-float ABI", it will be fixed later.
+; CHECK: Mismatch ABI. Currnet ABI is 'ilp32', but IR target-abi module flag is 'ilp32f'.
 
 define float @foo(i32 %a) nounwind #0 {
-; DEFAULT: # %bb.0:
-; DEFAULT-NEXT: fcvt.s.w fa0, a0
-; DEFAULT-NEXT: ret
-; RV32IF-ILP32F: # %bb.0:
-; RV32IF-ILP32F: fcvt.s.w fa0, a0
-; RV32IF-ILP32F: ret
   %conv = sitofp i32 %a to float
   ret float %conv
 }
 
 attributes #0 = { "target-features"="+f"}
 !llvm.module.flags = !{!0}
 !0 = !{i32 1, !"target-abi", !"ilp32f"}