[GlobalOpt] Fix global SRA incorrect alignment on some elements #115328
Conversation
This file contains hidden or 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
…e elements The logic had a flaw where the alignment from the original aggregate is unintentionally retained when the calculated known alignment is not higher than the ABI type alignment. Fixes llvm#115282
|
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-llvm-transforms Author: Bruno De Fraine (brunodf-snps) ChangesThe logic had a flaw where the alignment from the original aggregate is unintentionally retained for elements when the calculated known alignment is not higher than the element's ABI type alignment. Fixes #115282 Full diff: https://github.com/llvm/llvm-project/pull/115328.diff 4 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index 4647c65a5c850f..9aaa0212fc8445 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -575,15 +575,16 @@ static GlobalVariable *SRAGlobal(GlobalVariable *GV, const DataLayout &DL) {
*GV->getParent(), Ty, false, GlobalVariable::InternalLinkage,
Initializer, GV->getName() + "." + Twine(NameSuffix++), GV,
GV->getThreadLocalMode(), GV->getAddressSpace());
+ // Start out by copying attributes from the original, including alignment.
NGV->copyAttributesFrom(GV);
NewGlobals.insert({OffsetForTy, NGV});
// Calculate the known alignment of the field. If the original aggregate
- // had 256 byte alignment for example, something might depend on that:
+ // had 256 byte alignment for example, then the element at a given offset
+ // may also have a known alignment, and something might depend on that:
// propagate info to each field.
Align NewAlign = commonAlignment(StartAlignment, OffsetForTy);
- if (NewAlign > DL.getABITypeAlign(Ty))
- NGV->setAlignment(NewAlign);
+ NGV->setAlignment(std::max(NewAlign, DL.getABITypeAlign(Ty)));
// Copy over the debug info for the variable.
transferSRADebugInfo(GV, NGV, OffsetForTy * 8,
diff --git a/llvm/test/DebugInfo/X86/global-sra-struct-fit-segment.ll b/llvm/test/DebugInfo/X86/global-sra-struct-fit-segment.ll
index 94b7deec95f411..ab48ef2411f35c 100644
--- a/llvm/test/DebugInfo/X86/global-sra-struct-fit-segment.ll
+++ b/llvm/test/DebugInfo/X86/global-sra-struct-fit-segment.ll
@@ -20,7 +20,7 @@
%struct.BSS1 = type <{ [12 x i8] }>
;CHECK: @.BSS1.0 = internal unnamed_addr global i32 0, align 32, !dbg ![[GVE1:.*]]
-;CHECK: @.BSS1.1 = internal unnamed_addr global i32 0, align 32, !dbg ![[GVE2:.*]], !dbg ![[GVE4:.*]]
+;CHECK: @.BSS1.1 = internal unnamed_addr global i32 0, align 4, !dbg ![[GVE2:.*]], !dbg ![[GVE4:.*]]
;CHECK: @.BSS1.2 = internal unnamed_addr global i32 0, align 8, !dbg ![[GVE3:.*]]
@.BSS1 = internal global %struct.BSS1 zeroinitializer, align 32, !dbg !0, !dbg !7, !dbg !10, !dbg !27, !dbg !29
diff --git a/llvm/test/DebugInfo/X86/global-sra-struct-part-overlap-segment.ll b/llvm/test/DebugInfo/X86/global-sra-struct-part-overlap-segment.ll
index d7854cb822e3e9..9dd865edbbdadd 100644
--- a/llvm/test/DebugInfo/X86/global-sra-struct-part-overlap-segment.ll
+++ b/llvm/test/DebugInfo/X86/global-sra-struct-part-overlap-segment.ll
@@ -26,7 +26,7 @@
@.BSS3 = internal unnamed_addr global %struct.BSS3 zeroinitializer, align 32, !dbg !0, !dbg !7, !dbg !29
;CHECK: @.BSS3.0 = internal unnamed_addr global double 0.000000e+00, align 32, !dbg ![[GVE1:.*]], !dbg ![[GVE2:.*]]
-;CHECK: @.BSS3.1 = internal unnamed_addr global double 0.000000e+00, align 32, !dbg ![[GVE3:.*]], !dbg ![[GVE4:.*]], !dbg ![[GVE6:.*]]
+;CHECK: @.BSS3.1 = internal unnamed_addr global double 0.000000e+00, align 8, !dbg ![[GVE3:.*]], !dbg ![[GVE4:.*]], !dbg ![[GVE6:.*]]
;CHECK: @.BSS3.2 = internal unnamed_addr global double 0.000000e+00, align 16, !dbg ![[GVE5:.*]]
@.C363_mymod_bar_ = internal constant [2 x i8] c"IF"
diff --git a/llvm/test/Transforms/GlobalOpt/globalsra-align.ll b/llvm/test/Transforms/GlobalOpt/globalsra-align.ll
index aadd5b866a8224..74ccb3f279ed09 100644
--- a/llvm/test/Transforms/GlobalOpt/globalsra-align.ll
+++ b/llvm/test/Transforms/GlobalOpt/globalsra-align.ll
@@ -15,9 +15,9 @@ target datalayout = "p:16:32:64" ; 16-bit pointers with 32-bit ABI alignment and
;.
; CHECK: @[[A_4:[a-zA-Z0-9_$"\\.-]+]] = internal unnamed_addr externally_initialized global ptr null, align 8
-; CHECK: @[[A_5:[a-zA-Z0-9_$"\\.-]+]] = internal unnamed_addr externally_initialized global ptr null, align 16
+; CHECK: @[[A_5:[a-zA-Z0-9_$"\\.-]+]] = internal unnamed_addr externally_initialized global ptr null, align 8
; CHECK: @[[A_6:[a-zA-Z0-9_$"\\.-]+]] = internal unnamed_addr externally_initialized global ptr null, align 16
-; CHECK: @[[A_7:[a-zA-Z0-9_$"\\.-]+]] = internal unnamed_addr externally_initialized global ptr null, align 16
+; CHECK: @[[A_7:[a-zA-Z0-9_$"\\.-]+]] = internal unnamed_addr externally_initialized global ptr null, align 8
;.
define ptr @reduce_align_0() {
; CHECK-LABEL: @reduce_align_0(
@@ -31,7 +31,7 @@ define ptr @reduce_align_0() {
define ptr @reduce_align_1() {
; CHECK-LABEL: @reduce_align_1(
-; CHECK-NEXT: [[X:%.*]] = load ptr, ptr @a.5, align 16
+; CHECK-NEXT: [[X:%.*]] = load ptr, ptr @a.5, align 8
; CHECK-NEXT: ret ptr [[X]]
;
%x = load ptr, ptr getelementptr inbounds ([3 x [7 x ptr]], ptr @a, i64 0, i64 2, i64 1), align 4
@@ -51,7 +51,7 @@ define ptr @reduce_align_2() {
define ptr @reduce_align_3() {
; CHECK-LABEL: @reduce_align_3(
-; CHECK-NEXT: [[X:%.*]] = load ptr, ptr @a.7, align 16
+; CHECK-NEXT: [[X:%.*]] = load ptr, ptr @a.7, align 8
; CHECK-NEXT: ret ptr [[X]]
;
%x = load ptr, ptr getelementptr inbounds ([3 x [7 x ptr]], ptr @a, i64 0, i64 2, i64 3), align 4
|
|
I think the bug existed since commit 4796b4a. The updated expected alignments in Transforms/GlobalOpt/globalsra-align.ll revert the changes from that commit, with the exception of The tests DebugInfo/X86/global-sra-struct-fit-segment.ll and DebugInfo/X86/global-sra-struct-part-overlap-segment.ll are newer than the aforementioned commit. CC @nikic |
nikic
reviewed
Nov 7, 2024
Thanks. I still added a reduced test case with the example from the bug report. I suppose this should be pre-committed? In that case, you can commit 0a59028 first, and I'll rebase to remove it from this pull request. (Unless you think the effect is sufficiently clear from the other test cases and a dedicated test case is overkill; then I'll remove it again.) |
|
(ping) |
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.
The logic had a flaw where the alignment from the original aggregate is unintentionally retained for elements when the calculated known alignment is not higher than the element's ABI type alignment.
Fixes #115282