[llvm] [SeparateConstOffsetFromGEP] Fix sext/add overflow checks (PR #228562)
via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 2 12:50:15 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-backend-nvptx
Author: Alex MacLean (AlexMaclean)
<details>
<summary>Changes</summary>
Fix three cases where `SeparateConstOffsetFromGEP` distributes `sext` through an overflowing `add`:
- A nonnegative sum can still overflow when both operands are negative. Require at least one nonnegative operand. [Compiler Explorer](https://godbolt.org/z/WcPzWPTGq), [Alive2 counterexample](https://alive2.llvm.org/ce/z/Hsk_PE), [generalized proof](https://alive2.llvm.org/ce/z/EbgYFa).
- The bounds check used the GEP source type's size for every index. Track the exact index `Use` and use its element stride. [Compiler Explorer](https://godbolt.org/z/r9aGTsEE8), [Alive2 counterexample](https://alive2.llvm.org/ce/z/kqi9Cf), [generalized proof](https://alive2.llvm.org/ce/z/aAM4hM).
- Earlier GEP indices can move the pointer away from the object base. Restrict base-offset reasoning to the first index and use the object-size bound for later indices. [Compiler Explorer](https://godbolt.org/z/vjM19TKnY), [Alive2 counterexample](https://alive2.llvm.org/ce/z/FiHX4h), [generalized proof](https://alive2.llvm.org/ce/z/6E4kSv).
---
Patch is 28.98 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/228562.diff
3 Files Affected:
- (modified) llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp (+71-80)
- (added) llvm/test/Transforms/SeparateConstOffsetFromGEP/NVPTX/sext-add-inbounds.ll (+107)
- (added) llvm/test/Transforms/SeparateConstOffsetFromGEP/NVPTX/sext-add-nonnegative.ll (+139)
``````````diff
diff --git a/llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp b/llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp
index aa7219d3057f2..dd11ec2fae7f2 100644
--- a/llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp
+++ b/llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp
@@ -167,6 +167,7 @@
#include "llvm/Transforms/Utils/Local.h"
#include <cassert>
#include <cstdint>
+#include <iterator>
#include <optional>
#include <string>
@@ -206,19 +207,17 @@ class ConstantOffsetExtractor {
/// Extracts a constant offset from the given GEP index. It returns the
/// new index representing the remainder (equal to the original index minus
/// the constant offset), or nullptr if we cannot extract a constant offset.
- /// \p Idx The given GEP index
- /// \p GEP The given GEP
+ /// \p Idx The given GEP index use
/// \p UserChainTail Outputs the tail of UserChain so that we can
/// garbage-collect unused instructions in UserChain.
/// \p PreservesNUW Outputs whether the extraction allows preserving the
/// GEP's nuw flag, if it has one.
- static Value *Extract(Value *Idx, GetElementPtrInst *GEP,
- User *&UserChainTail, bool &PreservesNUW);
+ static Value *Extract(Use &Idx, User *&UserChainTail, bool &PreservesNUW);
/// Looks for a constant offset from the given GEP index without extracting
/// it. It returns the numeric value of the extracted constant offset (0 if
/// failed). The meaning of the arguments are the same as Extract.
- static APInt Find(Value *Idx, GetElementPtrInst *GEP);
+ static APInt Find(Use &Idx);
private:
ConstantOffsetExtractor(BasicBlock::iterator InsertionPt)
@@ -230,16 +229,13 @@ class ConstantOffsetExtractor {
/// otherwise, UserChain is empty.
///
/// \p V The given expression
- /// \p GEP The base GEP instruction, used for determining relevant
- /// types, flags, and non-negativity needed for safe
- /// reassociation
- /// \p Idx The original index of the GEP
+ /// \p Idx The original index use of the GEP, or nullptr if its
+ /// sign and bounds information no longer applies
/// \p SignExtended Whether V will be sign-extended in the computation of
/// the GEP index
/// \p ZeroExtended Whether V will be zero-extended in the computation of
/// the GEP index
- APInt find(Value *V, GetElementPtrInst *GEP, Value *Idx, bool SignExtended,
- bool ZeroExtended);
+ APInt find(Value *V, const Use *Idx, bool SignExtended, bool ZeroExtended);
/// A helper function to look into both operands of a binary operator.
APInt findInEitherOperand(BinaryOperator *BO, bool SignExtended,
@@ -293,11 +289,10 @@ class ConstantOffsetExtractor {
///
/// \p SignExtended Whether BO is surrounded by sext
/// \p ZeroExtended Whether BO is surrounded by zext
- /// \p GEP The base GEP instruction, used for determining relevant
- /// types and flags needed for safe reassociation.
- /// \p Idx The original index of the GEP
+ /// \p Idx The original index use of the GEP, or nullptr if its
+ /// sign and bounds information no longer applies
bool canTraceInto(bool SignExtended, bool ZeroExtended, BinaryOperator *BO,
- GetElementPtrInst *GEP, Value *Idx);
+ const Use *Idx);
/// Analyze a xor expression, and identify the bits in the constant operand
/// that are disjoint from the base operand's known set bits. For these
@@ -511,15 +506,15 @@ FunctionPass *llvm::createSeparateConstOffsetFromGEPPass(bool LowerGEP) {
// Checks if it is safe to reorder an add/sext result used in a GEP.
//
// An inbounds GEP does not guarantee that the index is non-negative.
-// This helper checks first if the index is known non-negative. If the index is
-// non-negative, the transform is always safe.
+// This helper checks first if the index and at least one add operand are known
+// non-negative. If so, the transform is safe.
// Second, it checks whether the GEP is inbounds and directly based on a global
// or an alloca, which are required to prove futher transform validity.
// If the GEP:
-// - Has a zero offset from the base, the index is non-negative (any negative
-// value would produce poison/UB)
+// - Has a zero offset from the base and Idx is its first index, the index is
+// non-negative (any negative value would produce poison/UB)
// - Has ObjectSize < (2^(N-1) - C + 1) * stride, where C is a constant from the
-// add, stride is the element size of Idx, and N is bitwidth of Idx.
+// add, stride is the element size of Idx, and N is bitwidth of the add.
// This is because with this pattern:
// %add = add iN %val, C
// %sext = sext iN %add to i64
@@ -538,12 +533,14 @@ FunctionPass *llvm::createSeparateConstOffsetFromGEPPass(bool LowerGEP) {
// producing a large positive value that still needs to be inbounds to the
// object size. If C is negative, we cannot make any useful assumptions based
// on the offset, since it would need to be extremely large.
-static bool canReorderAddSextToGEP(const GetElementPtrInst *GEP,
- const Value *Idx, const BinaryOperator *Add,
+static bool canReorderAddSextToGEP(const Use *Idx, const BinaryOperator *Add,
const DataLayout &DL) {
- if (isKnownNonNegative(Idx, DL))
+ if (isKnownNonNegative(Idx->get(), DL) &&
+ (isKnownNonNegative(Add->getOperand(0), DL) ||
+ isKnownNonNegative(Add->getOperand(1), DL)))
return true;
+ const auto *GEP = cast<GetElementPtrInst>(Idx->getUser());
if (!GEP->isInBounds())
return false;
@@ -562,7 +559,10 @@ static bool canReorderAddSextToGEP(const GetElementPtrInst *GEP,
// Calculate the threshold
APInt Threshold;
unsigned N = Add->getType()->getIntegerBitWidth();
- TypeSize ElemSize = DL.getTypeAllocSize(GEP->getSourceElementType());
+ auto GTI = gep_type_begin(GEP);
+ // Track the use: the same value may index different types in this GEP.
+ std::advance(GTI, Idx->getOperandNo() - 1);
+ TypeSize ElemSize = GTI.getSequentialElementStride(DL);
if (ElemSize.isScalable())
return false;
uint64_t Stride = ElemSize.getFixedValue();
@@ -578,8 +578,10 @@ static bool canReorderAddSextToGEP(const GetElementPtrInst *GEP,
APInt(128, Stride);
}
- if (Base && (isa<AllocaInst>(Base) || isa<GlobalObject>(Base)) &&
- !CI->isNegative()) {
+ // Only the first index is relative to Ptr. Earlier indices may move the
+ // pointer within the object, so later indices must use the object-size proof.
+ if (GTI == gep_type_begin(GEP) && Base &&
+ (isa<AllocaInst>(Base) || isa<GlobalObject>(Base)) && !CI->isNegative()) {
// If the offset is zero from an alloca or global, inbounds is sufficient to
// prove non-negativity if one add operand is non-negative
if (Offset == 0)
@@ -617,21 +619,17 @@ static bool canReorderAddSextToGEP(const GetElementPtrInst *GEP,
}
bool ConstantOffsetExtractor::canTraceInto(bool SignExtended, bool ZeroExtended,
- BinaryOperator *BO,
- GetElementPtrInst *GEP, Value *Idx) {
- // We only consider ADD, SUB and OR, because a non-zero constant found in
+ BinaryOperator *BO, const Use *Idx) {
+ // Do not trace into "or" unless it is equivalent to "add nuw nsw".
+ // This is the case if the or's disjoint flag is set.
+ if (BO->getOpcode() == Instruction::Or)
+ return cast<PossiblyDisjointInst>(BO)->isDisjoint();
+
+ // We only consider ADD and SUB here, because a non-zero constant found in
// expressions composed of these operations can be easily hoisted as a
// constant offset by reassociation.
if (BO->getOpcode() != Instruction::Add &&
- BO->getOpcode() != Instruction::Sub &&
- BO->getOpcode() != Instruction::Or) {
- return false;
- }
-
- // Do not trace into "or" unless it is equivalent to "add nuw nsw".
- // This is the case if the or's disjoint flag is set.
- if (BO->getOpcode() == Instruction::Or &&
- !cast<PossiblyDisjointInst>(BO)->isDisjoint())
+ BO->getOpcode() != Instruction::Sub)
return false;
// FIXME: We don't currently support constants from the RHS of subs,
@@ -651,7 +649,14 @@ bool ConstantOffsetExtractor::canTraceInto(bool SignExtended, bool ZeroExtended,
// 1 | 0 | sext(BO) == sext(A) op sext(B)
// 1 | 1 | zext(sext(BO)) ==
// | | zext(sext(A)) op zext(sext(B))
- if (BO->getOpcode() == Instruction::Add && !ZeroExtended && GEP) {
+ if (BO->getOpcode() == Instruction::Add && !ZeroExtended && Idx) {
+ const auto *GEP = cast<GetElementPtrInst>(Idx->getUser());
+ // For a sext(add nuw), allow tracing through when the enclosing GEP is both
+ // inbounds and nuw.
+ if (SignExtended && BO->hasNoUnsignedWrap() && GEP->isInBounds() &&
+ GEP->hasNoUnsignedWrap())
+ return true;
+
// If a + b >= 0 and (a >= 0 or b >= 0), then
// sext(a + b) = sext(a) + sext(b)
// even if the addition is not marked nsw.
@@ -660,27 +665,16 @@ bool ConstantOffsetExtractor::canTraceInto(bool SignExtended, bool ZeroExtended,
// index under certain conditions (see canReorderAddSextToGEP).
//
// Verified in @sext_add in split-gep.ll.
- if (canReorderAddSextToGEP(GEP, Idx, BO, DL))
+ if (canReorderAddSextToGEP(Idx, BO, DL))
return true;
}
- // For a sext(add nuw), allow tracing through when the enclosing GEP is both
- // inbounds and nuw.
- bool GEPInboundsNUW =
- GEP ? (GEP->isInBounds() && GEP->hasNoUnsignedWrap()) : false;
- if (BO->getOpcode() == Instruction::Add && SignExtended && !ZeroExtended &&
- GEPInboundsNUW && BO->hasNoUnsignedWrap())
- return true;
-
// sext (add/sub nsw A, B) == add/sub nsw (sext A), (sext B)
// zext (add/sub nuw A, B) == add/sub nuw (zext A), (zext B)
- if (BO->getOpcode() == Instruction::Add ||
- BO->getOpcode() == Instruction::Sub) {
- if (SignExtended && !BO->hasNoSignedWrap())
- return false;
- if (ZeroExtended && !BO->hasNoUnsignedWrap())
- return false;
- }
+ if (SignExtended && !BO->hasNoSignedWrap())
+ return false;
+ if (ZeroExtended && !BO->hasNoUnsignedWrap())
+ return false;
return true;
}
@@ -691,9 +685,10 @@ APInt ConstantOffsetExtractor::findInEitherOperand(BinaryOperator *BO,
// Save off the current height of the chain, in case we need to restore it.
size_t ChainLength = UserChain.size();
- // BO cannot use information from the base GEP at this point, so clear it.
+ // An intervening binary operator invalidates the GEP's index sign and bounds
+ // information, so do not pass it to either operand.
APInt ConstantOffset =
- find(BO->getOperand(0), nullptr, nullptr, SignExtended, ZeroExtended);
+ find(BO->getOperand(0), nullptr, SignExtended, ZeroExtended);
// If we found a constant offset in the left operand, stop and return that.
// This shortcut might cause us to miss opportunities of combining the
// constant offsets in both operands, e.g., (a + 4) + (b + 5) => (a + b) + 9.
@@ -705,8 +700,7 @@ APInt ConstantOffsetExtractor::findInEitherOperand(BinaryOperator *BO,
// since visiting the LHS didn't pan out.
UserChain.resize(ChainLength);
- ConstantOffset =
- find(BO->getOperand(1), nullptr, nullptr, SignExtended, ZeroExtended);
+ ConstantOffset = find(BO->getOperand(1), nullptr, SignExtended, ZeroExtended);
// If U is a sub operator, negate the constant offset found in the right
// operand.
if (BO->getOpcode() == Instruction::Sub)
@@ -719,8 +713,7 @@ APInt ConstantOffsetExtractor::findInEitherOperand(BinaryOperator *BO,
return ConstantOffset;
}
-APInt ConstantOffsetExtractor::find(Value *V, GetElementPtrInst *GEP,
- Value *Idx, bool SignExtended,
+APInt ConstantOffsetExtractor::find(Value *V, const Use *Idx, bool SignExtended,
bool ZeroExtended) {
// TODO(jingyue): We could trace into integer/pointer casts, such as
// inttoptr, ptrtoint, bitcast, and addrspacecast. We choose to handle only
@@ -737,7 +730,7 @@ APInt ConstantOffsetExtractor::find(Value *V, GetElementPtrInst *GEP,
ConstantOffset = CI->getValue();
} else if (BinaryOperator *BO = dyn_cast<BinaryOperator>(V)) {
// Trace into subexpressions for more hoisting opportunities.
- if (canTraceInto(SignExtended, ZeroExtended, BO, GEP, Idx))
+ if (canTraceInto(SignExtended, ZeroExtended, BO, Idx))
ConstantOffset = findInEitherOperand(BO, SignExtended, ZeroExtended);
else if (BO->getOpcode() == Instruction::Xor)
ConstantOffset = extractDisjointBitsFromXor(BO);
@@ -745,16 +738,15 @@ APInt ConstantOffsetExtractor::find(Value *V, GetElementPtrInst *GEP,
if (SignExtended || ZeroExtended)
return ConstantOffset;
ConstantOffset =
- find(U->getOperand(0), GEP, Idx, SignExtended, ZeroExtended)
- .trunc(BitWidth);
+ find(U->getOperand(0), Idx, SignExtended, ZeroExtended).trunc(BitWidth);
} else if (isa<SExtInst>(V)) {
ConstantOffset =
- find(U->getOperand(0), GEP, Idx, /* SignExtended */ true, ZeroExtended)
+ find(U->getOperand(0), Idx, /* SignExtended */ true, ZeroExtended)
.sext(BitWidth);
} else if (isa<ZExtInst>(V)) {
// As an optimization, we can clear the SignExtended flag because
// sext(zext(a)) = zext(a). Verified in @sext_zext in split-gep.ll.
- ConstantOffset = find(U->getOperand(0), GEP, Idx, /* SignExtended */ false,
+ ConstantOffset = find(U->getOperand(0), Idx, /* SignExtended */ false,
/* ZeroExtended */ true)
.zext(BitWidth);
}
@@ -976,20 +968,18 @@ static bool allowsPreservingNUW(const User *U) {
return true;
}
-static BasicBlock::iterator getIndexInsertionPoint(Value *Idx,
- GetElementPtrInst *GEP) {
- if (auto *I = dyn_cast<Instruction>(Idx))
+static BasicBlock::iterator getIndexInsertionPoint(const Use &Idx) {
+ if (auto *I = dyn_cast<Instruction>(Idx.get()))
if (auto IP = I->getInsertionPointAfterDef())
return *IP;
- return GEP->getIterator();
+ return cast<GetElementPtrInst>(Idx.getUser())->getIterator();
}
-Value *ConstantOffsetExtractor::Extract(Value *Idx, GetElementPtrInst *GEP,
- User *&UserChainTail,
+Value *ConstantOffsetExtractor::Extract(Use &Idx, User *&UserChainTail,
bool &PreservesNUW) {
- ConstantOffsetExtractor Extractor(getIndexInsertionPoint(Idx, GEP));
+ ConstantOffsetExtractor Extractor(getIndexInsertionPoint(Idx));
// Find a non-zero constant offset first.
- APInt ConstantOffset = Extractor.find(Idx, GEP, Idx, /* SignExtended */ false,
+ APInt ConstantOffset = Extractor.find(Idx, &Idx, /* SignExtended */ false,
/* ZeroExtended */ false);
if (ConstantOffset == 0) {
UserChainTail = nullptr;
@@ -1005,9 +995,10 @@ Value *ConstantOffsetExtractor::Extract(Value *Idx, GetElementPtrInst *GEP,
return IdxWithoutConstOffset;
}
-APInt ConstantOffsetExtractor::Find(Value *Idx, GetElementPtrInst *GEP) {
+APInt ConstantOffsetExtractor::Find(Use &Idx) {
+ auto *GEP = cast<GetElementPtrInst>(Idx.getUser());
return ConstantOffsetExtractor(GEP->getIterator())
- .find(Idx, GEP, Idx, /* SignExtended */ false, /* ZeroExtended */ false);
+ .find(Idx, &Idx, /* SignExtended */ false, /* ZeroExtended */ false);
}
bool SeparateConstOffsetFromGEP::canonicalizeArrayIndicesToIndexSize(
@@ -1021,7 +1012,7 @@ bool SeparateConstOffsetFromGEP::canonicalizeArrayIndicesToIndexSize(
if (GTI.isSequential()) {
if ((*I)->getType() != PtrIdxTy) {
*I = CastInst::CreateIntegerCast(*I, PtrIdxTy, true, "idxprom",
- getIndexInsertionPoint(*I, GEP));
+ getIndexInsertionPoint(*I));
Changed = true;
}
}
@@ -1045,7 +1036,7 @@ APInt SeparateConstOffsetFromGEP::accumulateByteOffset(GetElementPtrInst *GEP,
// Tries to extract a constant offset from this GEP index.
APInt ConstantOffset =
- ConstantOffsetExtractor::Find(GEP->getOperand(I), GEP)
+ ConstantOffsetExtractor::Find(GEP->getOperandUse(I))
.sextOrTrunc(IdxWidth);
if (ConstantOffset != 0) {
NeedsExtraction = true;
@@ -1279,8 +1270,8 @@ bool SeparateConstOffsetFromGEP::splitGEP(GetElementPtrInst *GEP) {
Value *Idx = GEP->getOperand(I);
User *UserChainTail;
bool PreservesNUW;
- Value *NewIdx = ConstantOffsetExtractor::Extract(Idx, GEP, UserChainTail,
- PreservesNUW);
+ Value *NewIdx = ConstantOffsetExtractor::Extract(
+ GEP->getOperandUse(I), UserChainTail, PreservesNUW);
if (NewIdx != nullptr) {
// Switches to the index with the constant offset removed.
GEP->setOperand(I, NewIdx);
diff --git a/llvm/test/Transforms/SeparateConstOffsetFromGEP/NVPTX/sext-add-inbounds.ll b/llvm/test/Transforms/SeparateConstOffsetFromGEP/NVPTX/sext-add-inbounds.ll
new file mode 100644
index 0000000000000..566650e1f83be
--- /dev/null
+++ b/llvm/test/Transforms/SeparateConstOffsetFromGEP/NVPTX/sext-add-inbounds.ll
@@ -0,0 +1,107 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 6
+; RUN: opt -S -mtriple=nvptx64-nvidia-cuda -passes=separate-const-offset-from-gep -verify-each -reassociate-geps-verify-no-dead-code < %s | FileCheck %s --check-prefixes=CHECK,SPLIT
+; RUN: opt -S -mtriple=nvptx64-nvidia-cuda -passes='separate-const-offset-from-gep<lower-gep>' -verify-each -reassociate-geps-verify-no-dead-code < %s | FileCheck %s --check-prefixes=CHECK,LOWER
+
+ at array = global [256 x i8] zeroinitializer
+
+; An earlier index can move the pointer away from the object base. For x = 127,
+; the sum wraps to -128 and the original GEP returns @array. Distributing sext
+; would instead return @array + 256. The first index is extracted before the
+; pass rewrites the second index, so it must not use the rewritten zero index
+; to infer that the second index is nonnegative.
+define ptr @preceding_index(i8 %x) {
+; SPLIT-LABEL: define ptr @preceding_index(
+; SPLIT-SAME: i8 [[X:%.*]]) {
+; SPLIT-NEXT: [[SUM:%.*]] = add i8 [[X]], 1
+; SPLIT-NEXT: [[INDEX:%.*]] = sext i8 [[SUM]] to i64
+; SPLIT-NEXT: [[TMP1:%.*]] = getelementptr [128 x i8], ptr @array, i64 0, i64 [[INDEX]]
+; SPLIT-NEXT: [[P1:%.*]] = getelementptr i8, ptr [[TMP1]], i64 128
+; SPLIT-NEXT: ret ptr [[P1]]
+;
+; LOWER-LABEL: define ptr @preceding_index(
+; LOWER-SAME: i8 [[X:%.*]]) {
+; LOWER-NEXT: [[SUM:%.*]] = add i8 [[X]], 1
+; LOWER-NEXT: [[INDEX:%.*]] = sext i8 [[SUM]] to i64
+; LOWER-NEXT: [[UGLYGEP:%.*]] = getelementptr i8, ptr @array, i64 [[INDEX]]
+; LOWER-NEXT: [[UGLYGEP1:%.*]] = getelementptr i8, ptr [[UGLYGEP]], i64 128
+; LOWER-NEXT: ret ptr [[UGLYGEP1]]
+;
+ %sum = add i8 %x, 1
+ %index = sext i8 %sum to i64
+ %p = getelementptr inbounds [128 x i8], ptr @array, i64 1, i64 %index
+ ret ptr %p
+}
+
+; The second index has stride 1, not 256. For x = -128, signed underflow gives
+; a valid offset of 127 into this object. Distributing sext would give -129.
+define ptr @negative_offset(i8 %x) {
+; CHECK-LABEL: define ptr @negative_offset(
+; CHECK-SAME: i8 [[X:%.*]]) {
+; CHECK-NEXT: [[SUM:%.*]] = add i8 [[X]], -1
+; CHECK-NEXT: [[INDEX:%.*]] = sext i8 [[SUM]] to i64
+; CHECK-NEXT: [[P:%.*]] = getelementptr inbounds [256 x i8], ptr @array, i64 0, i64 [[INDEX]]
+; CHECK-NEXT: ret ptr [[P]]
+;
+ %sum = add i8 %x, -1
+ %index = sext i8 %sum to i64
+ %p = getelementptr inbounds [256 x i8], ptr @array, i64 0, i64 %index
+ ret ptr %p
+}
+
+; The pointer starts 128 bytes into the object, so an index of -128 is valid.
+; Again, using the size of the en...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/228562
More information about the llvm-commits
mailing list