llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-backend-amdgpu Author: Simeon David Schaub (simeonschaub) <details> <summary>Changes</summary> We were observing miscompiles when using i128 in AMDGPU.jl (ref https://github.com/JuliaGPU/AMDGPU.jl/issues/1002#issuecomment-5156450276). Fix this by adding an appropriate entry to the data layout. Assisted-by: Claude Code (claude-opus-5) --- Full diff: https://github.com/llvm/llvm-project/pull/213523.diff 7 Files Affected: - (modified) clang/test/CodeGen/target-data.c (+2-2) - (modified) clang/test/CodeGenOpenCL/amdgpu-env-amdgcn.cl (+1-1) - (modified) llvm/lib/IR/AutoUpgrade.cpp (+6) - (modified) llvm/lib/TargetParser/TargetDataLayout.cpp (+8-2) - (added) llvm/test/CodeGen/AMDGPU/kernarg-i128-alignment.ll (+82) - (modified) llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp (+23-11) - (modified) mlir/test/Conversion/GPUToROCDL/gpu-to-rocdl.mlir (+1-1) ``````````diff diff --git a/clang/test/CodeGen/target-data.c b/clang/test/CodeGen/target-data.c index f2a09a40ee685..3bc58aa46ce69 100644 --- a/clang/test/CodeGen/target-data.c +++ b/clang/test/CodeGen/target-data.c @@ -160,12 +160,12 @@ // RUN: %clang_cc1 -triple amdgpu7.01-unknown -o - -emit-llvm %s \ // RUN: | FileCheck %s -check-prefix=R600SI -// R600SI: 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" +// R600SI: 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-i128:128-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" // Test default -target-cpu // RUN: %clang_cc1 -triple amdgpu-unknown -o - -emit-llvm %s \ // RUN: | FileCheck %s -check-prefix=R600SIDefault -// R600SIDefault: 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" +// R600SIDefault: 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-i128:128-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" // RUN: %clang_cc1 -triple arm64-unknown -o - -emit-llvm %s | \ // RUN: FileCheck %s -check-prefix=AARCH64 diff --git a/clang/test/CodeGenOpenCL/amdgpu-env-amdgcn.cl b/clang/test/CodeGenOpenCL/amdgpu-env-amdgcn.cl index 858a7db574f38..c39d22c16606d 100644 --- a/clang/test/CodeGenOpenCL/amdgpu-env-amdgcn.cl +++ b/clang/test/CodeGenOpenCL/amdgpu-env-amdgcn.cl @@ -1,5 +1,5 @@ // RUN: %clang_cc1 %s -O0 -triple amdgpu -emit-llvm -o - | FileCheck %s // RUN: %clang_cc1 %s -O0 -triple amdgpu---opencl -emit-llvm -o - | FileCheck %s -// CHECK: 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" +// CHECK: 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-i128:128-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" void foo(void) {} diff --git a/llvm/lib/IR/AutoUpgrade.cpp b/llvm/lib/IR/AutoUpgrade.cpp index 4502759417c5a..d0cb983fe8ed9 100644 --- a/llvm/lib/IR/AutoUpgrade.cpp +++ b/llvm/lib/IR/AutoUpgrade.cpp @@ -7163,6 +7163,12 @@ std::string llvm::UpgradeDataLayoutString(StringRef DL, StringRef TT) { Res.replace(Res.find(OldP8), OldP8.size(), "-p8:128:128:128:48-"); if (!DL.contains("-p9") && !DL.starts_with("p9")) Res.append("-p9:192:256:256:32"); + + // Add the alignment of i128, which used to be inherited from the i64 + // entry. Must come after the address space upgrades above, which rely on + // matching against the tail of the string. + if (!DL.contains("-i128") && !DL.starts_with("i128")) + Res.append("-i128:128"); } // Upgrade the ELF mangling mode. diff --git a/llvm/lib/TargetParser/TargetDataLayout.cpp b/llvm/lib/TargetParser/TargetDataLayout.cpp index 8b6f46642e4fa..1e74aa68cce16 100644 --- a/llvm/lib/TargetParser/TargetDataLayout.cpp +++ b/llvm/lib/TargetParser/TargetDataLayout.cpp @@ -273,10 +273,16 @@ static std::string computeAMDDataLayout(const Triple &TT) { // (address space 7), and 128-bit non-integral buffer resourcees (address // space 8) which cannot be non-trivilally accessed by LLVM memory operations // like getelementptr. + // + // i128 is aligned to 16 bytes to match the ABI implemented by Clang, whose + // AMDGPUTargetInfo leaves Int128Align at its 128-bit default. Without an + // explicit entry the alignment would be inherited from i64:64, and any + // frontend that lays out aggregates itself would disagree with the layout + // LLVM computes for the corresponding LLVM struct type. return "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"; + "i128:128-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"; } static std::string computeRISCVDataLayout(const Triple &TT, StringRef ABIName) { diff --git a/llvm/test/CodeGen/AMDGPU/kernarg-i128-alignment.ll b/llvm/test/CodeGen/AMDGPU/kernarg-i128-alignment.ll new file mode 100644 index 0000000000000..32e4dcb90e329 --- /dev/null +++ b/llvm/test/CodeGen/AMDGPU/kernarg-i128-alignment.ll @@ -0,0 +1,82 @@ +; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx1100 < %s | FileCheck %s + +; The AMDGPU data layout has to give i128 an ABI alignment of 16, matching the +; ABI implemented by Clang, whose AMDGPUTargetInfo leaves Int128Align at its +; 128-bit default. Without an explicit entry the alignment would be inherited +; from the i64:64 entry, and the layout LLVM computes for an aggregate would +; then disagree with the one a frontend used when it emitted the field offsets. +; +; For kernel arguments that disagreement is an ABI break rather than a missed +; optimization: the kernarg slot is sized from the data layout, so the tail of +; the argument is never copied into the kernarg segment and loads of it run off +; the end of the segment. + +; struct S { i64 a; i128 b; }: ABI align 16, size 32, offsetof(b) == 16. +; The byref slot must be 32 bytes, not 24, and `b` must be loaded from 0x20 +; (kernarg base 16 plus a field offset of 16) which stays inside the segment. +; CHECK-LABEL: {{^}}kernarg_i128: +; CHECK: s_load_b128 s[{{[0-9]+:[0-9]+}}], s[0:1], 0x20 +; CHECK: .amdhsa_kernarg_size 48 + +; An i128 following a smaller member is padded out to offset 16 rather than +; packed at offset 8. +; CHECK-LABEL: {{^}}kernarg_i128_after_i8: +; CHECK: s_load_b128 s[{{[0-9]+:[0-9]+}}], s[0:1], 0x20 +; CHECK: .amdhsa_kernarg_size 48 + +; A bare i128 kernel argument is 16-byte aligned in the kernarg segment, so it +; starts at 16 (not 8) and the argument after it at 32 (not 24). +; CHECK-LABEL: {{^}}kernarg_i128_scalar: +; CHECK: s_load_b128 s[{{[0-9]+:[0-9]+}}], s[0:1], 0x10 +; CHECK: .amdhsa_kernarg_size 40 + +; The kernel metadata is emitted once, after every function, so the per-kernel +; argument offsets are checked here in order rather than under each label. +; CHECK: .amdgpu_metadata + +; CHECK: .name: s +; CHECK-NEXT: .offset: 16 +; CHECK-NEXT: .size: 32 +; CHECK: .kernarg_segment_size: 48 +; CHECK: .name: kernarg_i128 +; +; CHECK: .name: s +; CHECK-NEXT: .offset: 16 +; CHECK-NEXT: .size: 32 +; CHECK: .kernarg_segment_size: 48 +; CHECK: .name: kernarg_i128_after_i8 +; +; CHECK: .name: a +; CHECK-NEXT: .offset: 16 +; CHECK-NEXT: .size: 16 +; CHECK: .name: b +; CHECK-NEXT: .offset: 32 +; CHECK-NEXT: .size: 8 +; CHECK: .kernarg_segment_size: 40 +; CHECK: .name: kernarg_i128_scalar + +define amdgpu_kernel void @kernarg_i128(ptr addrspace(1) %out, + ptr addrspace(4) byref({ i64, i128 }) align 16 %s) { + %pb = getelementptr inbounds i8, ptr addrspace(4) %s, i64 16 + %b = load i128, ptr addrspace(4) %pb, align 16 + store i128 %b, ptr addrspace(1) %out, align 16 + ret void +} + +define amdgpu_kernel void @kernarg_i128_after_i8(ptr addrspace(1) %out, + ptr addrspace(4) byref({ i8, i128 }) align 16 %s) { + %pb = getelementptr inbounds i8, ptr addrspace(4) %s, i64 16 + %b = load i128, ptr addrspace(4) %pb, align 16 + store i128 %b, ptr addrspace(1) %out, align 16 + ret void +} + +define amdgpu_kernel void @kernarg_i128_scalar(ptr addrspace(1) %out, i128 %a, i64 %b) { + %ext = zext i64 %b to i128 + %sum = add i128 %a, %ext + store i128 %sum, ptr addrspace(1) %out, align 16 + ret void +} + +!llvm.module.flags = !{!0} +!0 = !{i32 1, !"amdhsa_code_object_version", i32 500} diff --git a/llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp b/llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp index a082adbf6565e..49c5055076a59 100644 --- a/llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp +++ b/llvm/unittests/Bitcode/DataLayoutUpgradeTest.cpp @@ -43,18 +43,26 @@ TEST(DataLayoutUpgradeTest, ValidDataLayoutUpgrade) { // and that ANDGCN adds p7 and p8 as well. EXPECT_EQ(UpgradeDataLayoutString("e-p:64:64", "amdgcn"), "m:e-e-p:64:64-G1-ni:7:8:9-p7:160:256:256:32-p8:128:128:128:48-p9:" - "192:256:256:32"); + "192:256:256:32-i128:128"); EXPECT_EQ(UpgradeDataLayoutString("e-p:64:64-G1", "amdgcn"), "m:e-e-p:64:64-G1-ni:7:8:9-p7:160:256:256:32-p8:128:128:128:48-p9:" - "192:256:256:32"); + "192:256:256:32-i128:128"); // Check that the old AMDGCN p8:128:128 definition is upgraded EXPECT_EQ(UpgradeDataLayoutString("e-p:64:64-p8:128:128-G1", "amdgcn"), "m:e-e-p:64:64-p8:128:128:128:48-G1-ni:7:8:9-p7:160:256:256:32-p9:" - "192:256:256:32"); + "192:256:256:32-i128:128"); // but that r600 does not. EXPECT_EQ(UpgradeDataLayoutString("e-p:32:32-G1", "r600"), "m:e-e-p:32:32-G1"); + // Check that AMDGCN targets don't add an already declared i128 alignment, + // and that r600 never gains one. + EXPECT_EQ(UpgradeDataLayoutString("e-p:64:64-i128:64-G1", "amdgcn"), + "m:e-e-p:64:64-i128:64-G1-ni:7:8:9-p7:160:256:256:32-p8:128:128:128:" + "48-p9:192:256:256:32"); + EXPECT_EQ(UpgradeDataLayoutString("e-p:32:32-i64:64-G1", "r600"), + "m:e-e-p:32:32-i64:64-G1"); + // Ensure that the non-integral direction for address space 8 doesn't get // added in to pointer declarations. EXPECT_EQ( @@ -66,7 +74,7 @@ TEST(DataLayoutUpgradeTest, ValidDataLayoutUpgrade) { "m:e-e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32: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-p7:160:256:256:32-p8:128:128:" - "128:48-p9:192:256:256:32"); + "128:48-p9:192:256:256:32-i128:128"); // Check that SystemZ adds -S64 if needed. EXPECT_EQ(UpgradeDataLayoutString( @@ -158,24 +166,27 @@ TEST(DataLayoutUpgradeTest, NoDataLayoutUpgrade) { EXPECT_EQ(UpgradeDataLayoutString("G2", "r600"), "m:e-G2"); EXPECT_EQ(UpgradeDataLayoutString("e-p:64:64-G2", "amdgcn"), "m:e-e-p:64:64-G2-ni:7:8:9-p7:160:256:256:32-p8:128:128:128:48-p9:" - "192:256:256:32"); + "192:256:256:32-i128:128"); EXPECT_EQ(UpgradeDataLayoutString("G2-e-p:64:64", "amdgcn"), "m:e-G2-e-p:64:64-ni:7:8:9-p7:160:256:256:32-p8:128:128:128:48-p9:" - "192:256:256:32"); + "192:256:256:32-i128:128"); EXPECT_EQ(UpgradeDataLayoutString("e-p:64:64-G0", "amdgcn"), "m:e-e-p:64:64-G0-ni:7:8:9-p7:160:256:256:32-p8:128:128:128:48-p9:" - "192:256:256:32"); + "192:256:256:32-i128:128"); // Check that AMDGCN targets don't add already declared address space 7. EXPECT_EQ( UpgradeDataLayoutString("e-p:64:64-p7:64:64", "amdgcn"), - "m:e-e-p:64:64-p7:64:64-G1-ni:7:8:9-p8:128:128:128:48-p9:192:256:256:32"); + "m:e-e-p:64:64-p7:64:64-G1-ni:7:8:9-p8:128:128:128:48-p9:192:256:256:32-" + "i128:128"); EXPECT_EQ( UpgradeDataLayoutString("p7:64:64-G2-e-p:64:64", "amdgcn"), - "m:e-p7:64:64-G2-e-p:64:64-ni:7:8:9-p8:128:128:128:48-p9:192:256:256:32"); + "m:e-p7:64:64-G2-e-p:64:64-ni:7:8:9-p8:128:128:128:48-p9:192:256:256:32-" + "i128:128"); EXPECT_EQ( UpgradeDataLayoutString("e-p:64:64-p7:64:64-G1", "amdgcn"), - "m:e-e-p:64:64-p7:64:64-G1-ni:7:8:9-p8:128:128:128:48-p9:192:256:256:32"); + "m:e-e-p:64:64-p7:64:64-G1-ni:7:8:9-p8:128:128:128:48-p9:192:256:256:32-" + "i128:128"); // Check that SPIR & SPIRV targets don't add -G1 if there is already a -G // flag. @@ -218,7 +229,8 @@ TEST(DataLayoutUpgradeTest, EmptyDataLayout) { EXPECT_EQ(UpgradeDataLayoutString("", "r600"), "m:e-G1"); EXPECT_EQ( UpgradeDataLayoutString("", "amdgcn"), - "m:e-G1-ni:7:8:9-p7:160:256:256:32-p8:128:128:128:48-p9:192:256:256:32"); + "m:e-G1-ni:7:8:9-p7:160:256:256:32-p8:128:128:128:48-p9:192:256:256:32-" + "i128:128"); // Check that SPIR & SPIRV targets add G1 if it's not present. EXPECT_EQ(UpgradeDataLayoutString("", "spir"), "G1"); diff --git a/mlir/test/Conversion/GPUToROCDL/gpu-to-rocdl.mlir b/mlir/test/Conversion/GPUToROCDL/gpu-to-rocdl.mlir index 68a5328b8eb77..396f9bb63d84f 100755 --- a/mlir/test/Conversion/GPUToROCDL/gpu-to-rocdl.mlir +++ b/mlir/test/Conversion/GPUToROCDL/gpu-to-rocdl.mlir @@ -3,7 +3,7 @@ // RUN: mlir-opt %s -convert-gpu-to-rocdl='chipset=gfx950 index-bitwidth=32' -split-input-file | FileCheck --check-prefix=CHECK32 %s // CHECK-LABEL: @test_module -// CHECK-SAME: llvm.data_layout = "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" +// CHECK-SAME: llvm.data_layout = "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-i128:128-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" gpu.module @test_module { // CHECK-LABEL: func @gpu_index_ops() `````````` </details> https://github.com/llvm/llvm-project/pull/213523 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
