[LTO] Override TargetABI from module flags if present when creating TargetMachine #126497
Conversation
kito-cheng
requested review from
MaskRay,
mingmingl-llvm,
teresajohnson and
topperc
llvmbot
added
lld
lld:ELF
LTO
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-lld-elf Author: Kito Cheng (kito-cheng) Changes…argetMachine RISC-V's data layout is also determined by the ABI, not only the target triple, however the TargetMachine is created with the target triple's data layout, that is not always correct. This patch changes the TargetMachine's data layout to the one specified in the module flags if present. The same problem will happen with other targets like MIPS, but unfortunately, MIPS didn't emit the target-abi into the module flags, so this patch only fixes the issue for RISC-V. NOTE: MIPS with -mabi=n32 can trigger the same issue. Another possible solution is add new parameter to the TargetMachine constructor, but that would require changes in all the targets. Full diff: https://github.com/llvm/llvm-project/pull/126497.diff 2 Files Affected:
diff --git a/lld/test/ELF/lto/riscv-ilp32e.ll b/lld/test/ELF/lto/riscv-ilp32e.ll
new file mode 100644
index 000000000000000..ea2b57d48335865
--- /dev/null
+++ b/lld/test/ELF/lto/riscv-ilp32e.ll
@@ -0,0 +1,25 @@
+; REQUIRES: riscv
+;
+; Check that we don't crash on DataLayout incompatibility issue.
+;
+; RUN: llvm-as %s -o %t.o
+; RUN: ld.lld %t.o -o %t.elf
+; RUN: llvm-readobj -h %t.elf | FileCheck %s --check-prefixes=CHECK
+;
+; CHECK: Machine: EM_RISCV (0xF3)
+; CHECK: EF_RISCV_RVE (0x8)
+
+
+target datalayout = "e-m:e-p:32:32-i64:64-n32-S32"
+target triple = "riscv32-unknown-unknown-elf"
+
+define dso_local i32 @_start() #0 {
+entry:
+ ret i32 0
+}
+
+attributes #0 = { "target-cpu"="generic-rv32" "target-features"="+32bit,+e" }
+
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 1, !"target-abi", !"ilp32e"}
diff --git a/llvm/lib/LTO/LTOBackend.cpp b/llvm/lib/LTO/LTOBackend.cpp
index 8a2dddce4892c1a..b9d2242d4a2cf46 100644
--- a/llvm/lib/LTO/LTOBackend.cpp
+++ b/llvm/lib/LTO/LTOBackend.cpp
@@ -221,8 +221,18 @@ createTargetMachine(const Config &Conf, const Target *TheTarget, Module &M) {
else
CodeModel = M.getCodeModel();
+ TargetOptions TargetOpts = Conf.Options;
+
+ if (TargetOpts.MCOptions.ABIName.empty()) {
+ Metadata *ABI = M.getModuleFlag("target-abi");
+ if (ABI) {
+ const StringRef &ModABI = cast<MDString>(ABI)->getString();
+ TargetOpts.MCOptions.ABIName = ModABI;
+ }
+ }
+
std::unique_ptr<TargetMachine> TM(TheTarget->createTargetMachine(
- TheTriple, Conf.CPU, Features.getString(), Conf.Options, RelocModel,
+ TheTriple, Conf.CPU, Features.getString(), TargetOpts, RelocModel,
CodeModel, Conf.CGOptLevel));
assert(TM && "Failed to create target machine");
|
teresajohnson
left a comment
teresajohnson
left a comment
There was a problem hiding this comment.
Given that this isn't an lld change, the test should be in LLVM not lld. Take a look at llvm/test/LTO/X86/codemodel-1.ll for an example, and the LTO subdirectory presumably needs a RISCV subdirectory.
|
Changes:
|
|
I read the description here, and was a little concerned, but on reading the code I'm not.
This sentence concerned me, but reading the implementation, it isn't quite what's happening. Instead, you're providing the Not only is the existing datalayout logic unaffected, but i think this now brings how target-abi is handled inline with how datalayout is handled:
It would be good to update the message when this lands to clarify this, as my first reading did have me concerned we didn't follow this logic which could have affected a lot of tests and other users of serialised LLVM IR.
I looked at this a while ago (iirc for the same problem) and it's really really messy to fix. We should do it sometime, but I recognise we cannot do it quickly. This is a better step for right now. |
|
@kito-cheng I'd mentioned in the last RISC-V sync-up call that I was seeing warnings when linking e.g. SPEC in the llvm-test-suite and Sam mentioned this patch. I was wondering to what degree there are known issues you're tracking (and if there is a bug report for this?). Trialling this patch, I actually see more warnings emitted when linking spec 2017. (Obviously, that doesn't necessarily indicate a problem with the patch - we could just be surfacing more underlying issues). Is this expected? (EDIT: It's not immediately obvious to me why this patch results in more warnings than before, but I assume the warnings are #69780 and perhaps fixed by #100833) Before: After: Build config: |
|
@lenary comment updated, it should look like more reasonable :P |
I poked at it a bit more and added a comment here with a reproducer, as it seems that issue is acting as a meta-bug of sorts. If you have thoughts on this it would be very welcome. So although this PR is nothing to do with that problem I isolated, I'll note again that it does result in additional warnings when building the llvm-test-suite vs before it was applied - so I wonder if that's expected? |
|
Looks like #100833 has been closed without commit for now. |
MaskRay
changed the title
teresajohnson
left a comment
teresajohnson
left a comment
There was a problem hiding this comment.
lgtm once the existing suggestions from other reviewers are resolved
…argetMachine RISC-V's data layout is also determined by the ABI, not only the target triple, however the TargetMachine is created with the target triple's data layout, that is not always correct. This patch changes the TargetMachine's data layout to the one specified in the module flags if present. The same problem will happen with other targets like MIPS, but unfortunately, MIPS didn't emit the target-abi into the module flags, so this patch only fixes the issue for RISC-V. NOTE: MIPS with -mabi=n32 can trigger the same issue. Another possible solution is add new parameter to the TargetMachine constructor, but that would require changes in all the targets.
kito-cheng
force-pushed
the
kitoc/fix-lto-datalayout-computation
branch
from
d747098 to
1d61381
Compare
Co-authored-by: Alexander Richardson <[email protected]>
8 participants
…argetMachine
RISC-V's data layout is determined by the ABI, not just the target triple. However, the TargetMachine is created using the data layout from the target triple, which is not always correct. This patch uses the target ABI from the module and passes it to the TargetMachine, ensuring that the data layout is set correctly according to the ABI.
The same problem will happen with other targets like MIPS, but unfortunately, MIPS didn't emit the target-abi into the module flags, so this patch only fixes the issue for RISC-V.
NOTE: MIPS with -mabi=n32 can trigger the same issue.
Another possible solution is add new parameter to the TargetMachine constructor, but that would require changes in all the targets.