[AMDPGU] Auto-upgrade ELF mangling in the data layout #163644

jhuber6 · 2025-10-15T21:42:58Z

Summary:
The changes in #163011 caused
all ELF platforms to default to ELF mangling. We want to auto upgrade
this for linking in new programs to old ones.

llvmbot · 2025-10-15T21:43:32Z

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-lto

Author: Joseph Huber (jhuber6)

Changes

Summary:
Recent changes caused all AMDGPU toolchain programs to emit unhelpful
warnings since we linke a lot of external libraries. For now just accept
the old triple without warnings. We can probably remove this in the
future when these get updated.

Full diff: https://github.com/llvm/llvm-project/pull/163644.diff

3 Files Affected:

(modified) llvm/lib/Linker/IRMover.cpp (+7)
(added) llvm/test/Linker/Inputs/legacy-amdgcn.ll (+2)
(added) llvm/test/Linker/amdgcn-triple.ll (+6)

diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp
index 1bff6cd25156d..6b84fea4a4952 100644
--- a/llvm/lib/Linker/IRMover.cpp
+++ b/llvm/lib/Linker/IRMover.cpp
@@ -1489,6 +1489,13 @@ Error IRLinker::run() {
                                   SrcTriple.getOSName() == "unknown");
     EnableTripleWarning = !SrcHasLibDeviceTriple;
     EnableDLWarning = !(SrcHasLibDeviceTriple && SrcHasLibDeviceDL);
+  } else if (SrcTriple.isAMDGPU() && DstTriple.isAMDGPU()) {
+    StringRef SrcDL = SrcM->getDataLayoutStr();
+    StringRef DstDL = DstM.getDataLayoutStr();
+
+    // Suppress the data layout warning if the old layout without ELF mangling
+    // specified is used.
+    EnableDLWarning = DstDL.drop_front(4) != SrcDL;
   }
 
   if (EnableDLWarning && (SrcM->getDataLayout() != DstM.getDataLayout())) {
diff --git a/llvm/test/Linker/Inputs/legacy-amdgcn.ll b/llvm/test/Linker/Inputs/legacy-amdgcn.ll
new file mode 100644
index 0000000000000..19d622d5f887f
--- /dev/null
+++ b/llvm/test/Linker/Inputs/legacy-amdgcn.ll
@@ -0,0 +1,2 @@
+target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128:128:48-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-G1-ni:7:8:9"
+target triple = "amdgcn-amd-amdhsa"
diff --git a/llvm/test/Linker/amdgcn-triple.ll b/llvm/test/Linker/amdgcn-triple.ll
new file mode 100644
index 0000000000000..9244277d1a1b0
--- /dev/null
+++ b/llvm/test/Linker/amdgcn-triple.ll
@@ -0,0 +1,6 @@
+; RUN: llvm-link %s %S/Inputs/legacy-amdgcn.ll -S -o - 2>&1 | FileCheck %s
+
+; CHECK-NOT: Linking two modules of different data layouts
+; CHECK: target triple = "amdgcn-amd-amdhsa"
+target datalayout = "e-m:e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-p7:160:256:256:32-p8:128:128:128:48-p9:192:256:256:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-G1-ni:7:8:9"
+target triple = "amdgcn-amd-amdhsa"

shiltian · 2025-10-16T13:46:43Z

I'm not sure if we can do this in IR auto upgrade instead of suppressing the warning?

jhuber6 · 2025-10-16T13:51:27Z

I'm not sure if we can do this in IR auto upgrade instead of suppressing the warning?

IR Linker auto upgrades are currently applied after this check I believe, but I suppose we could add one beforehand? Not sure if that's correct either because it is a legitimately different data layout with different semantics. This mostly just suppresses the warning because there's nothing the user can do about it and it's unreasonable for an empty .cpp file to fill the entire screen with warnings.

shiltian · 2025-10-16T14:01:31Z

because it is a legitimately different data layout with different semantics

This depends because in this particular case it doesn't actually change anything, and this is also the whole point of IR upgrade.

I don't prefer to suppress any warning if it is not the only solution but I'm also open to other opinions.

jhuber6 · 2025-10-16T14:03:55Z

I don't prefer to suppress any warning if it is not the only solution but I'm also open to other opinions.

We do the same for the CUDA devlice libs, I could hard code the module names to a list if you'd prefer.

shiltian · 2025-10-16T14:08:42Z

I guess at least we might want it to be more robust, like to drop the entire ELF mangling specification and then compare, instead of just cut off the first 4 chars.

jhuber6 · 2025-10-16T14:10:30Z

I guess at least we might want it to be more robust, like to drop the entire ELF mangling specification and then compare, instead of just cut off the first 4 chars.

That's exactly what dropping the first 4 chars does, I was first going to check if they're the same minus the mangling but the DataLayout doesn't expose that and I figured it wasn't worth modifying the class for this dumb use-case.

nikic · 2025-10-16T14:14:04Z

This should be handled in UpgradeDataLayoutString(), not ad-hoc in IRMover.

jhuber6 · 2025-10-16T14:16:11Z

UpgradeDataLayoutString

Didn't know that existed, certainly makes things more straightforward. Thanks.

b-sumner · 2025-10-16T14:24:13Z

Generally we just update the four .ll files in the device library. Are there other libraries we're concerned about now where a warning wouldn't be desired?

jhuber6 · 2025-10-16T14:25:30Z

Generally we just update the four .ll files in the device library. Are there other libraries we're concerned about now where a warning wouldn't be desired?

It's all the files with a data layout, and if we changed this I suppose there should be auto-upgrade anyway.

shiltian · 2025-10-16T14:27:48Z

An alternative can be just to do what NVPTX is doing, which is just to compare the key specs that we care about most, since we only support one "mode" for anything specified in data layout anyway, though I'm not sure if that's the right thing to do.

b-sumner · 2025-10-16T14:46:08Z

Generally we just update the four .ll files in the device library. Are there other libraries we're concerned about now where a warning wouldn't be desired?

It's all the files with a data layout, and if we changed this I suppose there should be auto-upgrade anyway.

Aren't there only 4 files? Isn't the warning a good thing in general?

jhuber6 · 2025-10-16T14:51:12Z

Generally we just update the four .ll files in the device library. Are there other libraries we're concerned about now where a warning wouldn't be desired?

It's all the files with a data layout, and if we changed this I suppose there should be auto-upgrade anyway.

Aren't there only 4 files? Isn't the warning a good thing in general?

OpenMP currently uses these, HIP has ockl in addition I believe.

 "-mlink-builtin-bitcode" "/opt/rocm/amdgcn/bitcode/ocml.bc"
 "-mlink-builtin-bitcode" "/opt/rocm/amdgcn/bitcode/oclc_daz_opt_off.bc"
 "-mlink-builtin-bitcode" "/opt/rocm/amdgcn/bitcode/oclc_unsafe_math_off.bc"
 "-mlink-builtin-bitcode" "/opt/rocm/amdgcn/bitcode/oclc_finite_only_off.bc"
 "-mlink-builtin-bitcode" "/opt/rocm/amdgcn/bitcode/oclc_correctly_rounded_sqrt_on.bc"
 "-mlink-builtin-bitcode" "/opt/rocm/amdgcn/bitcode/oclc_wavefrontsize64_off.bc"
 "-mlink-builtin-bitcode" "/opt/rocm/amdgcn/bitcode/oclc_isa_version_1030.bc"
 "-mlink-builtin-bitcode" "/opt/rocm/amdgcn/bitcode/oclc_abi_version_600.bc"

jhuber6 · 2025-10-16T15:12:38Z

This applies to all the data layouts, I could make it only apply to this very specific one if necessary, but I'm assuming this is what we want.

b-sumner · 2025-10-16T15:15:05Z

Generally we just update the four .ll files in the device library. Are there other libraries we're concerned about now where a warning wouldn't be desired?

It's all the files with a data layout, and if we changed this I suppose there should be auto-upgrade anyway.

Aren't there only 4 files? Isn't the warning a good thing in general?

OpenMP currently uses these, HIP has ockl in addition I believe.
 "-mlink-builtin-bitcode" "/opt/rocm/amdgcn/bitcode/ocml.bc"
...

For those, I'd say the warning is exposing a problem which should be fixed by updating the 4 .ll files in the device library,

jhuber6 · 2025-10-16T15:20:47Z

For those, I'd say the warning is exposing a problem which should be fixed by updating the 4 .ll files in the device library,

Those libraries are external to LLVM so it will take a long time for users to get the newer version.

b-sumner · 2025-10-16T16:18:41Z

For those, I'd say the warning is exposing a problem which should be fixed by updating the 4 .ll files in the device library,

Those libraries are external to LLVM so it will take a long time for users to get the newer version.

I don't think that's a great reason to disable this warning in the compiler.

jhuber6 · 2025-10-16T16:19:53Z

I don't think that's a great reason to disable this warning in the compiler.

There's tons of existing auto-upgrading for AMDGPU already, I don't think this is that different but I'm not an expert on the existing handling.

nikic · 2025-10-16T16:24:47Z

All data layout changes are generally expected to come with an auto-upgrade.

llvm/lib/IR/AutoUpgrade.cpp

shiltian · 2025-10-16T19:20:24Z

It seems like m:e is the spec for ELF mangling that was added in #163011. Should we check the existence of that, and then append -m:e or prepend m:e- instead?

jhuber6 · 2025-10-16T19:21:47Z

It seems like m:e is the spec for ELF mangling that was added in #163011. Should we check the existence of that, and then append -m:e or prepend m:e- instead?

Definitely, I was actually just reading up on the Data Layout semantics and remembered that - is the separator and not :.

Summary: The changes in llvm#163011 caused all ELF platforms to default to ELF mangling. We want to auto upgrade this for linking in new programs to old ones. contains

jhuber6 requested a review from arsenm October 15, 2025 21:42

llvmbot added the LTO Link time optimization (regular/full LTO or ThinLTO) label Oct 15, 2025

jhuber6 requested review from aengelke, b-sumner, shiltian and slinder1 October 16, 2025 13:06

jhuber6 force-pushed the legacy_warn branch from f4f613b to 132cf3a Compare October 16, 2025 15:11

llvmbot added the llvm:ir label Oct 16, 2025

jhuber6 changed the title ~~[AMDGPU] Suppress DataLayout warnings after recent change~~ [AMDPGU] Auto-upgrade ELF mangling in the data layout Oct 16, 2025

jhuber6 requested a review from AlexVlx October 16, 2025 16:20

shiltian reviewed Oct 16, 2025

View reviewed changes

llvm/lib/IR/AutoUpgrade.cpp Outdated Show resolved Hide resolved

[AMDPGU] Auto-upgrade ELF mangling in the data layout

c1e1390

Summary: The changes in llvm#163011 caused all ELF platforms to default to ELF mangling. We want to auto upgrade this for linking in new programs to old ones. contains

jhuber6 force-pushed the legacy_warn branch from bd13d58 to c1e1390 Compare October 16, 2025 19:33

shiltian approved these changes Oct 16, 2025

View reviewed changes

jhuber6 merged commit 728e925 into llvm:main Oct 17, 2025
10 checks passed

[AMDPGU] Auto-upgrade ELF mangling in the data layout #163644

[AMDPGU] Auto-upgrade ELF mangling in the data layout #163644

Uh oh!

Conversation

jhuber6 commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shiltian commented Oct 16, 2025

Uh oh!

jhuber6 commented Oct 16, 2025

Uh oh!

shiltian commented Oct 16, 2025

Uh oh!

jhuber6 commented Oct 16, 2025

Uh oh!

shiltian commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhuber6 commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Oct 16, 2025

Uh oh!

jhuber6 commented Oct 16, 2025

Uh oh!

b-sumner commented Oct 16, 2025

Uh oh!

jhuber6 commented Oct 16, 2025

Uh oh!

shiltian commented Oct 16, 2025

Uh oh!

b-sumner commented Oct 16, 2025

Uh oh!

jhuber6 commented Oct 16, 2025

Uh oh!

jhuber6 commented Oct 16, 2025

Uh oh!

b-sumner commented Oct 16, 2025

Uh oh!

jhuber6 commented Oct 16, 2025

Uh oh!

b-sumner commented Oct 16, 2025

Uh oh!

jhuber6 commented Oct 16, 2025

Uh oh!

nikic commented Oct 16, 2025

Uh oh!

Uh oh!

shiltian commented Oct 16, 2025

Uh oh!

jhuber6 commented Oct 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jhuber6 commented Oct 15, 2025 •

edited

Loading

llvmbot commented Oct 15, 2025 •

edited

Loading

shiltian commented Oct 16, 2025 •

edited

Loading

jhuber6 commented Oct 16, 2025 •

edited

Loading