[SeparateConstOffsetFromGEP] Check if non-extracted indices may be negative when preserving inbounds #159515
+18
−17
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
…gative when preserving inbounds If we know that the initial GEP was inbounds, and we change it to a sequence of GEPs from the same base pointer where every offset is non-negative, then the new GEPs are inbounds. So far, the implementation only checked if the extracted offsets are non-negative. In cases where non-extracted offsets can be negative, this would cause the inbounds flag to be wrongly preserved. Fixes an issue in #130617 found by nikic.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
ritter-x2a
requested review from
AlexMaclean,
arsenm,
jmmartinez,
jrbyrnes and
nikic
ritter-x2a
added
llvm:transforms
backend:AMDGPU
backend:NVPTX
labels
Graphite App
|
@llvm/pr-subscribers-backend-nvptx @llvm/pr-subscribers-backend-amdgpu Author: Fabian Ritter (ritter-x2a) ChangesIf we know that the initial GEP was inbounds, and we change it to a sequence of Fixes an issue in #130617 found by nikic. Full diff: https://github.com/llvm/llvm-project/pull/159515.diff 4 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp b/llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp
index 8a5569743ab44..333cbb6ed1384 100644
--- a/llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp
+++ b/llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp
@@ -1116,22 +1116,23 @@ bool SeparateConstOffsetFromGEP::splitGEP(GetElementPtrInst *GEP) {
// Splits this GEP index into a variadic part and a constant offset, and
// uses the variadic part as the new index.
- Value *OldIdx = GEP->getOperand(I);
+ Value *Idx = GEP->getOperand(I);
User *UserChainTail;
bool PreservesNUW;
- Value *NewIdx = ConstantOffsetExtractor::Extract(
- OldIdx, GEP, UserChainTail, PreservesNUW);
+ Value *NewIdx = ConstantOffsetExtractor::Extract(Idx, GEP, UserChainTail,
+ PreservesNUW);
if (NewIdx != nullptr) {
// Switches to the index with the constant offset removed.
GEP->setOperand(I, NewIdx);
// After switching to the new index, we can garbage-collect UserChain
// and the old index if they are not used.
RecursivelyDeleteTriviallyDeadInstructions(UserChainTail);
- RecursivelyDeleteTriviallyDeadInstructions(OldIdx);
- AllOffsetsNonNegative =
- AllOffsetsNonNegative && isKnownNonNegative(NewIdx, *DL);
+ RecursivelyDeleteTriviallyDeadInstructions(Idx);
+ Idx = NewIdx;
AllNUWPreserved &= PreservesNUW;
}
+ AllOffsetsNonNegative =
+ AllOffsetsNonNegative && isKnownNonNegative(Idx, *DL);
}
}
if (ExtractBase) {
diff --git a/llvm/test/Transforms/SeparateConstOffsetFromGEP/AMDGPU/reorder-gep-inbounds.ll b/llvm/test/Transforms/SeparateConstOffsetFromGEP/AMDGPU/reorder-gep-inbounds.ll
index 5cb3ee6f72e3b..f7c019bba5d1f 100644
--- a/llvm/test/Transforms/SeparateConstOffsetFromGEP/AMDGPU/reorder-gep-inbounds.ll
+++ b/llvm/test/Transforms/SeparateConstOffsetFromGEP/AMDGPU/reorder-gep-inbounds.ll
@@ -279,8 +279,8 @@ define void @addrspace3(ptr addrspace(3) %in.ptr, i64 %in.idx1) {
; CHECK-NEXT: entry:
; CHECK-NEXT: [[IN_IDX1_NNEG:%.*]] = and i64 [[IN_IDX1]], 9223372036854775807
; CHECK-NEXT: [[IDXPROM:%.*]] = trunc i64 [[IN_IDX1_NNEG]] to i32
-; CHECK-NEXT: [[TMP0:%.*]] = getelementptr inbounds i128, ptr addrspace(3) [[IN_PTR]], i32 [[IDXPROM]]
-; CHECK-NEXT: [[IDX11:%.*]] = getelementptr inbounds i8, ptr addrspace(3) [[TMP0]], i32 1024
+; CHECK-NEXT: [[TMP0:%.*]] = getelementptr i128, ptr addrspace(3) [[IN_PTR]], i32 [[IDXPROM]]
+; CHECK-NEXT: [[IDX11:%.*]] = getelementptr i8, ptr addrspace(3) [[TMP0]], i32 1024
; CHECK-NEXT: ret void
;
entry:
@@ -296,8 +296,8 @@ define void @addrspace7(ptr addrspace(7) %in.ptr, i64 %in.idx1) {
; CHECK-NEXT: entry:
; CHECK-NEXT: [[IN_IDX1_NNEG:%.*]] = and i64 [[IN_IDX1]], 9223372036854775807
; CHECK-NEXT: [[IDXPROM:%.*]] = trunc i64 [[IN_IDX1_NNEG]] to i32
-; CHECK-NEXT: [[TMP0:%.*]] = getelementptr inbounds i128, ptr addrspace(7) [[IN_PTR]], i32 [[IDXPROM]]
-; CHECK-NEXT: [[IDX11:%.*]] = getelementptr inbounds i8, ptr addrspace(7) [[TMP0]], i32 1024
+; CHECK-NEXT: [[TMP0:%.*]] = getelementptr i128, ptr addrspace(7) [[IN_PTR]], i32 [[IDXPROM]]
+; CHECK-NEXT: [[IDX11:%.*]] = getelementptr i8, ptr addrspace(7) [[TMP0]], i32 1024
; CHECK-NEXT: ret void
;
entry:
diff --git a/llvm/test/Transforms/SeparateConstOffsetFromGEP/NVPTX/gep-chain.ll b/llvm/test/Transforms/SeparateConstOffsetFromGEP/NVPTX/gep-chain.ll
index e4b9dac72e96c..bbfab0f96fc53 100644
--- a/llvm/test/Transforms/SeparateConstOffsetFromGEP/NVPTX/gep-chain.ll
+++ b/llvm/test/Transforms/SeparateConstOffsetFromGEP/NVPTX/gep-chain.ll
@@ -24,11 +24,11 @@ define i32 @more_interesting(ptr %ptr, i32 %offset1, i32 %offset2) {
; CHECK-NEXT: [[IDXPROM:%.*]] = sext i32 [[OFFSET1]] to i64
; CHECK-NEXT: [[GEP1:%.*]] = getelementptr inbounds [[STRUCT_UCHAR4:%.*]], ptr [[PTR]], i64 [[IDXPROM]]
; CHECK-NEXT: [[IDXPROM1:%.*]] = sext i32 [[OFFSET2]] to i64
-; CHECK-NEXT: [[TMP1:%.*]] = getelementptr inbounds [[STRUCT_UCHAR4]], ptr [[GEP1]], i64 [[IDXPROM1]]
-; CHECK-NEXT: [[TMP2:%.*]] = getelementptr inbounds i8, ptr [[TMP1]], i64 8
+; CHECK-NEXT: [[TMP1:%.*]] = getelementptr [[STRUCT_UCHAR4]], ptr [[GEP1]], i64 [[IDXPROM1]]
+; CHECK-NEXT: [[TMP2:%.*]] = getelementptr i8, ptr [[TMP1]], i64 8
; CHECK-NEXT: [[V1:%.*]] = load i32, ptr [[TMP2]], align 4
; CHECK-NEXT: [[IDXPROM2:%.*]] = sext i32 [[OFFSET2]] to i64
-; CHECK-NEXT: [[TMP4:%.*]] = getelementptr inbounds [[STRUCT_UCHAR4]], ptr [[TMP1]], i64 [[IDXPROM2]]
+; CHECK-NEXT: [[TMP4:%.*]] = getelementptr [[STRUCT_UCHAR4]], ptr [[TMP1]], i64 [[IDXPROM2]]
; CHECK-NEXT: [[V2:%.*]] = load i32, ptr [[TMP4]], align 4
; CHECK-NEXT: [[R:%.*]] = add i32 [[V1]], [[V2]]
; CHECK-NEXT: ret i32 [[R]]
diff --git a/llvm/test/Transforms/SeparateConstOffsetFromGEP/NVPTX/split-gep.ll b/llvm/test/Transforms/SeparateConstOffsetFromGEP/NVPTX/split-gep.ll
index da04a6e979425..77b3434f4f159 100644
--- a/llvm/test/Transforms/SeparateConstOffsetFromGEP/NVPTX/split-gep.ll
+++ b/llvm/test/Transforms/SeparateConstOffsetFromGEP/NVPTX/split-gep.ll
@@ -372,8 +372,8 @@ define ptr @trunk_explicit(ptr %ptr, i64 %idx) {
; CHECK-LABEL: define ptr @trunk_explicit(
; CHECK-SAME: ptr [[PTR:%.*]], i64 [[IDX:%.*]]) {
; CHECK-NEXT: entry:
-; CHECK-NEXT: [[TMP0:%.*]] = getelementptr inbounds [[STRUCT0:%.*]], ptr [[PTR]], i64 0, i32 3, i64 [[IDX]], i32 1
-; CHECK-NEXT: [[PTR21:%.*]] = getelementptr inbounds i8, ptr [[TMP0]], i64 3216
+; CHECK-NEXT: [[TMP0:%.*]] = getelementptr [[STRUCT0:%.*]], ptr [[PTR]], i64 0, i32 3, i64 [[IDX]], i32 1
+; CHECK-NEXT: [[PTR21:%.*]] = getelementptr i8, ptr [[TMP0]], i64 3216
; CHECK-NEXT: ret ptr [[PTR21]]
;
entry:
@@ -389,8 +389,8 @@ define ptr @trunk_long_idx(ptr %ptr, i64 %idx) {
; CHECK-LABEL: define ptr @trunk_long_idx(
; CHECK-SAME: ptr [[PTR:%.*]], i64 [[IDX:%.*]]) {
; CHECK-NEXT: entry:
-; CHECK-NEXT: [[TMP0:%.*]] = getelementptr inbounds [[STRUCT0:%.*]], ptr [[PTR]], i64 0, i32 3, i64 [[IDX]], i32 1
-; CHECK-NEXT: [[PTR21:%.*]] = getelementptr inbounds i8, ptr [[TMP0]], i64 3216
+; CHECK-NEXT: [[TMP0:%.*]] = getelementptr [[STRUCT0:%.*]], ptr [[PTR]], i64 0, i32 3, i64 [[IDX]], i32 1
+; CHECK-NEXT: [[PTR21:%.*]] = getelementptr i8, ptr [[TMP0]], i64 3216
; CHECK-NEXT: ret ptr [[PTR21]]
;
entry:
|
krzysz00
approved these changes
Sep 18, 2025
Merge activity
|
ritter-x2a
deleted the
users/ritter-x2a/09-18-_separateconstoffsetfromgep_check_if_non-extracted_indices_may_be_negative_when_preserving_inbounds
branch
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/27482 Here is the relevant piece of the build log for the reference |
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.
If we know that the initial GEP was inbounds, and we change it to a sequence of
GEPs from the same base pointer where every offset is non-negative, then the
new GEPs are inbounds. So far, the implementation only checked if the extracted
offsets are non-negative. In cases where non-extracted offsets can be negative,
this would cause the inbounds flag to be wrongly preserved.
Fixes an issue in #130617 found by nikic.