This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by khchen on May 16 2021, 9:44 AM.

Details

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

Diff Detail

Event Timeline

khchen created this revision.May 16 2021, 9:44 AM
khchen requested review of this revision.May 16 2021, 9:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2021, 9:44 AM
jrtc27 added inline comments.May 16 2021, 9:51 AM
llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
95–115

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

jrtc27 added inline comments.May 16 2021, 9:58 AM
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 updated this revision to Diff 345738.May 16 2021, 7:27 PM
khchen marked 2 inline comments as done.
  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 updated this revision to Diff 345850.May 17 2021, 6:04 AM

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

khchen retitled this revision from [PoC][RISCV] Report an error when target-abi option is empty but target-abi module flag is not. to [PoC][RISCV] Report an error when ABI mismatch with target-abi module flag..May 17 2021, 6:09 AM
khchen edited the summary of this revision. (Show Details)
khchen retitled this revision from [PoC][RISCV] Report an error when ABI mismatch with target-abi module flag. to [RISCV] Report an error when ABI mismatch with target-abi module flag..May 17 2021, 7:21 AM
craig.topper added inline comments.May 17 2021, 6:24 PM
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

craig.topper edited the summary of this revision. (Show Details)May 17 2021, 6:25 PM
craig.topper edited the summary of this revision. (Show Details)
khchen updated this revision to Diff 346046.May 17 2021, 10:38 PM
khchen marked 2 inline comments as done.

address Craig's comments, thanks!

khchen updated this revision to Diff 347356.May 24 2021, 5:32 AM
  1. Handle an empty module flag.

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

  1. Add empty module flag test and combine all tests into one. It is simiar to compress.ll
luismarques added inline comments.May 26 2021, 3:07 PM
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 updated this revision to Diff 348177.May 27 2021, 12:08 AM
khchen marked an inline comment as done.

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 added inline comments.May 27 2021, 5:38 AM
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 updated this revision to Diff 348870.May 31 2021, 7:49 PM

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

Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2021, 7:49 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
noah95 added a subscriber: noah95.Jul 1 2021, 10:55 PM
jrtc27 added inline comments.Jul 4 2021, 8:09 PM
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 added inline comments.Jul 5 2021, 12:03 AM
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 added inline comments.Jul 6 2021, 4:15 PM
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 updated this revision to Diff 357420.Jul 8 2021, 11:12 PM

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

khchen abandoned this revision.May 23 2023, 9:35 AM

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

evandro removed a subscriber: evandro.May 23 2023, 3:07 PM