-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[ConstraintElim] Bail out on non-canonical GEPs #156688
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-llvm-transforms Author: Yingwei Zheng (dtcxzyw) ChangesIn most cases, GEPs should be canonicalized by InstCombine. Bail out on non-canonical forms for simplicity. Full diff: https://github.com/llvm/llvm-project/pull/156688.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
index 1b4d8c786cbb2..4acc3f2d84690 100644
--- a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
@@ -1125,6 +1125,12 @@ static bool getConstraintFromMemoryAccess(GetElementPtrInst &GEP,
if (Offset.VariableOffsets.size() != 1)
return false;
+ uint64_t BitWidth = Offset.ConstantOffset.getBitWidth();
+ auto &[Index, Scale] = Offset.VariableOffsets.front();
+ // Bail out on non-canonical GEPs.
+ if (Index->getType()->getScalarSizeInBits() != BitWidth)
+ return false;
+
ObjectSizeOpts Opts;
// Workaround for gep inbounds, ptr null, idx.
Opts.NullIsUnknownSize = true;
@@ -1140,8 +1146,6 @@ static bool getConstraintFromMemoryAccess(GetElementPtrInst &GEP,
// With nuw flag, we know that the index addition doesn't have unsigned wrap.
// If (AllocSize - (ConstOffset + AccessSize)) wraps around, there is no valid
// value for Index.
- uint64_t BitWidth = Offset.ConstantOffset.getBitWidth();
- auto &[Index, Scale] = Offset.VariableOffsets.front();
APInt MaxIndex = (APInt(BitWidth, Size->getFixedValue() - AccessSize,
/*isSigned=*/false, /*implicitTrunc=*/true) -
Offset.ConstantOffset)
diff --git a/llvm/test/Transforms/ConstraintElimination/implied-by-bounded-memory-access.ll b/llvm/test/Transforms/ConstraintElimination/implied-by-bounded-memory-access.ll
index 8e3862b5714d0..90caf52d04164 100644
--- a/llvm/test/Transforms/ConstraintElimination/implied-by-bounded-memory-access.ll
+++ b/llvm/test/Transforms/ConstraintElimination/implied-by-bounded-memory-access.ll
@@ -371,3 +371,21 @@ define i8 @load_from_null(i64 %idx) {
%add = add i8 %load, %zext
ret i8 %add
}
+
+define i8 @load_global_non_canonical_gep(i32 %idx) {
+; CHECK-LABEL: define i8 @load_global_non_canonical_gep(
+; CHECK-SAME: i32 [[IDX:%.*]]) {
+; CHECK-NEXT: [[GEP:%.*]] = getelementptr nuw i8, ptr @g, i32 [[IDX]]
+; CHECK-NEXT: [[LOAD:%.*]] = load i8, ptr [[GEP]], align 1
+; CHECK-NEXT: [[CMP:%.*]] = icmp ult i32 [[IDX]], 5
+; CHECK-NEXT: [[ZEXT:%.*]] = zext i1 [[CMP]] to i8
+; CHECK-NEXT: [[ADD:%.*]] = add i8 [[LOAD]], [[ZEXT]]
+; CHECK-NEXT: ret i8 [[ADD]]
+;
+ %gep = getelementptr nuw i8, ptr @g, i32 %idx
+ %load = load i8, ptr %gep
+ %cmp = icmp ult i32 %idx, 5
+ %zext = zext i1 %cmp to i8
+ %add = add i8 %load, %zext
+ ret i8 %add
+}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is something of a footgun, possibly we should make collectOffset() reject these...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
Thanks! |
In most cases, GEPs should be canonicalized by InstCombine. Bail out on non-canonical forms for simplicity.
Fixes #155253 (comment).
If we really need to handle non-canonical forms, see https://alive2.llvm.org/ce/z/RTwsFT.