[SPARC] Align i128 to 16 bytes in SPARC datalayouts #106951
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Align i128s to 16 bytes, following the example at https://reviews.llvm.org/D86310. clang already does this, but do it in backend code too for the benefit of other frontends (see e.g llvm#102783).
llvmbot
added
clang
|
CC @beetrees |
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-clang Author: Koakuma (koachan) ChangesAlign i128s to 16 bytes, following the example at https://reviews.llvm.org/D86310. clang already does this implicitly, but do it in backend code too for the benefit of other frontends (see e.g #102783 & rust-lang/rust#128950). Full diff: https://github.com/llvm/llvm-project/pull/106951.diff 4 Files Affected:
diff --git a/clang/lib/Basic/Targets/Sparc.h b/clang/lib/Basic/Targets/Sparc.h
index 3357bee33e1ac7..ee0d3e2b4329eb 100644
--- a/clang/lib/Basic/Targets/Sparc.h
+++ b/clang/lib/Basic/Targets/Sparc.h
@@ -151,7 +151,7 @@ class LLVM_LIBRARY_VISIBILITY SparcV8TargetInfo : public SparcTargetInfo {
public:
SparcV8TargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
: SparcTargetInfo(Triple, Opts) {
- resetDataLayout("E-m:e-p:32:32-i64:64-f128:64-n32-S64");
+ resetDataLayout("E-m:e-p:32:32-i64:64-i128:128-f128:64-n32-S64");
// NetBSD / OpenBSD use long (same as llvm default); everyone else uses int.
switch (getTriple().getOS()) {
default:
@@ -188,7 +188,7 @@ class LLVM_LIBRARY_VISIBILITY SparcV8elTargetInfo : public SparcV8TargetInfo {
public:
SparcV8elTargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
: SparcV8TargetInfo(Triple, Opts) {
- resetDataLayout("e-m:e-p:32:32-i64:64-f128:64-n32-S64");
+ resetDataLayout("e-m:e-p:32:32-i64:64-i128:128-f128:64-n32-S64");
}
};
@@ -198,7 +198,7 @@ class LLVM_LIBRARY_VISIBILITY SparcV9TargetInfo : public SparcTargetInfo {
SparcV9TargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
: SparcTargetInfo(Triple, Opts) {
// FIXME: Support Sparc quad-precision long double?
- resetDataLayout("E-m:e-i64:64-n32:64-S128");
+ resetDataLayout("E-m:e-i64:64-i128:128-n32:64-S128");
// This is an LP64 platform.
LongWidth = LongAlign = PointerWidth = PointerAlign = 64;
diff --git a/clang/test/CodeGen/target-data.c b/clang/test/CodeGen/target-data.c
index 41cbd5a0219d5e..8548aa00cfe877 100644
--- a/clang/test/CodeGen/target-data.c
+++ b/clang/test/CodeGen/target-data.c
@@ -28,11 +28,11 @@
// RUN: %clang_cc1 -triple sparc-sun-solaris -emit-llvm -o - %s | \
// RUN: FileCheck %s --check-prefix=SPARC-V8
-// SPARC-V8: target datalayout = "E-m:e-p:32:32-i64:64-f128:64-n32-S64"
+// SPARC-V8: target datalayout = "E-m:e-p:32:32-i64:64-i128:128-f128:64-n32-S64"
// RUN: %clang_cc1 -triple sparcv9-sun-solaris -emit-llvm -o - %s | \
// RUN: FileCheck %s --check-prefix=SPARC-V9
-// SPARC-V9: target datalayout = "E-m:e-i64:64-n32:64-S128"
+// SPARC-V9: target datalayout = "E-m:e-i64:64-i128:128-n32:64-S128"
// RUN: %clang_cc1 -triple mipsel-linux-gnu -o - -emit-llvm %s | \
// RUN: FileCheck %s -check-prefix=MIPS-32EL
diff --git a/llvm/lib/Target/Sparc/SparcTargetMachine.cpp b/llvm/lib/Target/Sparc/SparcTargetMachine.cpp
index fec2d3a35ae6d2..50a96368bbdca9 100644
--- a/llvm/lib/Target/Sparc/SparcTargetMachine.cpp
+++ b/llvm/lib/Target/Sparc/SparcTargetMachine.cpp
@@ -48,6 +48,10 @@ static std::string computeDataLayout(const Triple &T, bool is64Bit) {
// Alignments for 64 bit integers.
Ret += "-i64:64";
+ // Alignments for 128 bit integers.
+ // This is not specified in the ABI document but is the de facto standard.
+ Ret += "-i128:128";
+
// On SparcV9 128 floats are aligned to 128 bits, on others only to 64.
// On SparcV9 registers can hold 64 or 32 bits, on others only 32.
if (is64Bit)
diff --git a/llvm/test/CodeGen/SPARC/data-align.ll b/llvm/test/CodeGen/SPARC/data-align.ll
new file mode 100644
index 00000000000000..d4a39524da44f6
--- /dev/null
+++ b/llvm/test/CodeGen/SPARC/data-align.ll
@@ -0,0 +1,27 @@
+; RUN: llc < %s -march=sparc | FileCheck %s
+; RUN: llc < %s -march=sparcel | FileCheck %s
+; RUN: llc < %s -march=sparcv9 | FileCheck %s
+
+; CHECK: .Li8:
+; CHECK-DAG: .size .Li8, 1
+@i8 = private constant i8 42
+
+; CHECK: .p2align 1
+; CHECK-NEXT: .Li16:
+; CHECK-DAG: .size .Li16, 2
+@i16 = private constant i16 42
+
+; CHECK: .p2align 2
+; CHECK-NEXT: .Li32:
+; CHECK-DAG: .size .Li32, 4
+@i32 = private constant i32 42
+
+; CHECK: .p2align 3
+; CHECK-NEXT: .Li64:
+; CHECK-DAG: .size .Li64, 8
+@i64 = private constant i64 42
+
+; CHECK: .p2align 4
+; CHECK-NEXT: .Li128:
+; CHECK-DAG: .size .Li128, 16
+@i128 = private constant i128 42
|
|
I guess this is a breaking change? |
Yes, but as previously discussed, it's acceptable breakage.
Yes, this should be handled in UpgradeDataLayoutString(). |
|
Ping? |
nikic
reviewed
Sep 19, 2024
nikic
reviewed
Sep 22, 2024
llvm/lib/IR/AutoUpgrade.cpp
Outdated
| std::string I128 = "-i128:128"; | ||
| if (StringRef Ref = Res; !Ref.contains(I128)) { | ||
| SmallVector<StringRef, 4> Groups; | ||
| Regex R("^([Ee](-[mpi][^-]*)*)((-[^mpi][^-]*)*)$"); |
There was a problem hiding this comment.
I think it would be easier to match for -i64:64- here? It looks like that part doesn't vary across subtargets, so we don't need more generic matching.
nikic
approved these changes
Sep 29, 2024
llvm/lib/IR/AutoUpgrade.cpp
Outdated
| // Add "-i128:128" | ||
| std::string I64 = "-i64:64"; | ||
| std::string I128 = "-i128:128"; | ||
| if (StringRef Ref = Res; !Ref.contains(I128)) { |
There was a problem hiding this comment.
Suggested change
| if (StringRef Ref = Res; !Ref.contains(I128)) { | |
| if (!StringRef(Res).contains(I128)) { |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/13/builds/2592 Here is the relevant piece of the build log for the reference |
|
In a local and several more instances of that. I wonder how this patch has ever been tested natively... |
|
FWIW, the same failures exist on Linux/sparc64, too. |
|
My bad, yeah, I forgot to run bugpoint tests. |
3 tasks
koachan
added a commit
that referenced
this pull request
Oct 2, 2024
It turns out that we cannot rely on the presence of `-i64:64` as a position reference when adding the `-i128:128` datalayout string due to some custom datalayout strings lacking it (e.g ones used by bugpoint, among other things). Do not add the `-i128:128` string in that case. This fixes the regression introduced in #106951.
xgupta
pushed a commit
to xgupta/llvm-project
that referenced
this pull request
Oct 4, 2024
Align i128s to 16 bytes, following the example at https://reviews.llvm.org/D86310. clang already does this implicitly, but do it in backend code too for the benefit of other frontends (see e.g llvm#102783 & rust-lang/rust#128950).
xgupta
pushed a commit
to xgupta/llvm-project
that referenced
this pull request
Oct 4, 2024
It turns out that we cannot rely on the presence of `-i64:64` as a position reference when adding the `-i128:128` datalayout string due to some custom datalayout strings lacking it (e.g ones used by bugpoint, among other things). Do not add the `-i128:128` string in that case. This fixes the regression introduced in llvm#106951.
maurer
added a commit
to maurer/rust
that referenced
this pull request
Oct 31, 2024
LLVM continues to align more 128-bit integers to 128-bits in the data layout rather than relying on the high level language to do it. Update SPARC target files to match and add a backcompat replacement for current LLVMs. See llvm/llvm-project#106951 for details
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Nov 1, 2024
llvm: Match new LLVM 128-bit integer alignment on sparc LLVM continues to align more 128-bit integers to 128-bits in the data layout rather than relying on the high level language to do it. Update SPARC target files to match and add a backcompat replacement for current LLVMs. See llvm/llvm-project#106951 for details `@rustbot` label: +llvm-main r? `@durin42` (Please wait for the LLVM CI to come back before approving), creating this PR to get it tested there.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Nov 1, 2024
Rollup merge of rust-lang#132422 - maurer:sparc-layout, r=durin42 llvm: Match new LLVM 128-bit integer alignment on sparc LLVM continues to align more 128-bit integers to 128-bits in the data layout rather than relying on the high level language to do it. Update SPARC target files to match and add a backcompat replacement for current LLVMs. See llvm/llvm-project#106951 for details `@rustbot` label: +llvm-main r? `@durin42` (Please wait for the LLVM CI to come back before approving), creating this PR to get it tested there.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Align i128s to 16 bytes, following the example at https://reviews.llvm.org/D86310.
clang already does this implicitly, but do it in backend code too for the benefit of other frontends (see e.g #102783 & rust-lang/rust#128950).