[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

7–9

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 ↗

(On Diff #345850)

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–7

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/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>(
+
+    // The taget-abi option should equal to 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.
+    StringRef OptABIName = Options.MCOptions.getABIName();
+    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();
+      if (OptABIName != ModuleABIName)
+        errs() << "Inconsistent ABI string representation. -target-abi option "
+                  "is '" +
+                      OptABIName + "' but IR target-abi module flag is '" +
+                      ModuleABIName + "'.\n";
     }
-    I = std::make_unique<RISCVSubtarget>(TargetTriple, CPU, TuneCPU, FS, ABIName, *this);
+    I = std::make_unique<RISCVSubtarget>(TargetTriple, CPU, TuneCPU, FS,
+                                         OptABIName, *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
+; Need to specific -target-abi option although the rv32 default ABI is
+; ilp32 which eqaul to target-abi module flag.
+; RUN: llc -mtriple=riscv32 < %s -o /dev/null 2>&1 | FileCheck %s
+; RUN: llc -mtriple=riscv32 -target-abi ilp32f < %s -o /dev/null 2>&1 \
+; RUN:   | FileCheck -check-prefix=MISMATCH %s
 
-; RV32IF-ILP32F: -target-abi option != target-abi module flag
-
-; FLAGS: Flags: 0x0
+; CHECK: Inconsistent ABI string representation. -target-abi option is '' but IR target-abi module flag is 'ilp32'.
+; MISMATCH: Inconsistent ABI string representation. -target-abi option 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
+; Need to specific -target-abi option because target-abi module flag is not empty
+; 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 -check-prefix=MISMATCH %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: Inconsistent ABI string representation. -target-abi option is '' but IR target-abi module flag is 'ilp32f'.
+; MISMATCH: Inconsistent ABI string representation. -target-abi option 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"}