Skip to content

Commit

Permalink
[InstSimplify] Remove incorrect icmp of gep fold (PR52429)
Browse files Browse the repository at this point in the history 
As described in https://bugs.llvm.org/show_bug.cgi?id=52429 this
fold is incorrect, because inbounds only guarantees that the
pointers don't wrap in the unsigned space: It is possible that
the sign boundary is crossed by an object.

I'm dropping the fold entirely rather than adjusting it, because
computePointerICmp() fully subsumes it (just with correct predicate
handling).

Differential Revision: https://reviews.llvm.org/D113343
  • Loading branch information
@nikic
nikic committed Nov 6, 2021
1 parent 859a6d9 commit e3cec17
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 30 deletions.
24 changes: 0 additions & 24 deletions llvm/lib/Analysis/InstructionSimplify.cpp
View file
Expand Up @@ -3665,30 +3665,6 @@ static Value *SimplifyICmpInst(unsigned Predicate, Value *LHS, Value *RHS,
CRHS->getPointerOperand(), Q))
return C;

if (GetElementPtrInst *GLHS = dyn_cast<GetElementPtrInst>(LHS)) {
if (GEPOperator *GRHS = dyn_cast<GEPOperator>(RHS)) {
if (GLHS->getPointerOperand() == GRHS->getPointerOperand() &&
GLHS->hasAllConstantIndices() && GRHS->hasAllConstantIndices() &&
(ICmpInst::isEquality(Pred) ||
(GLHS->isInBounds() && GRHS->isInBounds() &&
Pred == ICmpInst::getSignedPredicate(Pred)))) {
// The bases are equal and the indices are constant. Build a constant
// expression GEP with the same indices and a null base pointer to see
// what constant folding can make out of it.
Constant *Null = Constant::getNullValue(GLHS->getPointerOperandType());
SmallVector<Value *, 4> IndicesLHS(GLHS->indices());
Constant *NewLHS = ConstantExpr::getGetElementPtr(
GLHS->getSourceElementType(), Null, IndicesLHS);

SmallVector<Value *, 4> IndicesRHS(GRHS->indices());
Constant *NewRHS = ConstantExpr::getGetElementPtr(
GLHS->getSourceElementType(), Null, IndicesRHS);
Constant *NewICmp = ConstantExpr::getICmp(Pred, NewLHS, NewRHS);
return ConstantFoldConstant(NewICmp, Q.DL);
}
}
}

// If the comparison is with the result of a select instruction, check whether
// comparing with either branch of the select always yields the same value.
if (isa<SelectInst>(LHS) || isa<SelectInst>(RHS))
Expand Down
11 changes: 9 additions & 2 deletions llvm/test/Transforms/InstCombine/icmp-custom-dl.ll
View file
Expand Up @@ -159,9 +159,13 @@ define i1 @test61_as1(i8 addrspace(1)* %foo, i16 %i, i16 %j) {
; Don't transform non-inbounds GEPs.
}

; Negative test: GEP inbounds may cross sign boundary.
define i1 @test62(i8* %a) {
; CHECK-LABEL: @test62(
; CHECK-NEXT: ret i1 true
; CHECK-NEXT: [[ARRAYIDX1:%.*]] = getelementptr inbounds i8, i8* [[A:%.*]], i32 1
; CHECK-NEXT: [[ARRAYIDX2:%.*]] = getelementptr inbounds i8, i8* [[A]], i32 10
; CHECK-NEXT: [[CMP:%.*]] = icmp slt i8* [[ARRAYIDX1]], [[ARRAYIDX2]]
; CHECK-NEXT: ret i1 [[CMP]]
;
%arrayidx1 = getelementptr inbounds i8, i8* %a, i64 1
%arrayidx2 = getelementptr inbounds i8, i8* %a, i64 10
Expand All @@ -171,7 +175,10 @@ define i1 @test62(i8* %a) {

define i1 @test62_as1(i8 addrspace(1)* %a) {
; CHECK-LABEL: @test62_as1(
; CHECK-NEXT: ret i1 true
; CHECK-NEXT: [[ARRAYIDX1:%.*]] = getelementptr inbounds i8, i8 addrspace(1)* [[A:%.*]], i16 1
; CHECK-NEXT: [[ARRAYIDX2:%.*]] = getelementptr inbounds i8, i8 addrspace(1)* [[A]], i16 10
; CHECK-NEXT: [[CMP:%.*]] = icmp slt i8 addrspace(1)* [[ARRAYIDX1]], [[ARRAYIDX2]]
; CHECK-NEXT: ret i1 [[CMP]]
;
%arrayidx1 = getelementptr inbounds i8, i8 addrspace(1)* %a, i64 1
%arrayidx2 = getelementptr inbounds i8, i8 addrspace(1)* %a, i64 10
Expand Down
11 changes: 9 additions & 2 deletions llvm/test/Transforms/InstCombine/icmp.ll
View file
Expand Up @@ -1129,9 +1129,13 @@ define void @test58() {
}
declare i32 @test58_d(i64)

; Negative test: GEP inbounds may cross sign boundary.
define i1 @test62(i8* %a) {
; CHECK-LABEL: @test62(
; CHECK-NEXT: ret i1 true
; CHECK-NEXT: [[ARRAYIDX1:%.*]] = getelementptr inbounds i8, i8* [[A:%.*]], i64 1
; CHECK-NEXT: [[ARRAYIDX2:%.*]] = getelementptr inbounds i8, i8* [[A]], i64 10
; CHECK-NEXT: [[CMP:%.*]] = icmp slt i8* [[ARRAYIDX1]], [[ARRAYIDX2]]
; CHECK-NEXT: ret i1 [[CMP]]
;
%arrayidx1 = getelementptr inbounds i8, i8* %a, i64 1
%arrayidx2 = getelementptr inbounds i8, i8* %a, i64 10
Expand All @@ -1141,7 +1145,10 @@ define i1 @test62(i8* %a) {

define i1 @test62_as1(i8 addrspace(1)* %a) {
; CHECK-LABEL: @test62_as1(
; CHECK-NEXT: ret i1 true
; CHECK-NEXT: [[ARRAYIDX1:%.*]] = getelementptr inbounds i8, i8 addrspace(1)* [[A:%.*]], i16 1
; CHECK-NEXT: [[ARRAYIDX2:%.*]] = getelementptr inbounds i8, i8 addrspace(1)* [[A]], i16 10
; CHECK-NEXT: [[CMP:%.*]] = icmp slt i8 addrspace(1)* [[ARRAYIDX1]], [[ARRAYIDX2]]
; CHECK-NEXT: ret i1 [[CMP]]
;
%arrayidx1 = getelementptr inbounds i8, i8 addrspace(1)* %a, i64 1
%arrayidx2 = getelementptr inbounds i8, i8 addrspace(1)* %a, i64 10
Expand Down
8 changes: 6 additions & 2 deletions llvm/test/Transforms/InstSimplify/compare.ll
View file
Expand Up @@ -108,7 +108,7 @@ define i1 @gep6(%gept* %x) {
define i1 @gep7(%gept* %x) {
; CHECK-LABEL: @gep7(
; CHECK-NEXT: [[A:%.*]] = getelementptr [[GEPT:%.*]], %gept* [[X:%.*]], i64 0, i32 0
; CHECK-NEXT: [[EQUAL:%.*]] = icmp eq i32* [[A]], getelementptr (%gept, %gept* @gepz, i32 0, i32 0)
; CHECK-NEXT: [[EQUAL:%.*]] = icmp eq i32* [[A]], getelementptr ([[GEPT]], %gept* @gepz, i32 0, i32 0)
; CHECK-NEXT: ret i1 [[EQUAL]]
;
%a = getelementptr %gept, %gept* %x, i64 0, i32 0
Expand Down Expand Up @@ -294,9 +294,13 @@ define i1 @gep17() {
ret i1 %cmp
}

; Negative test: GEP inbounds may cross sign boundary.
define i1 @gep_same_base_constant_indices(i8* %a) {
; CHECK-LABEL: @gep_same_base_constant_indices(
; CHECK-NEXT: ret i1 true
; CHECK-NEXT: [[ARRAYIDX1:%.*]] = getelementptr inbounds i8, i8* [[A:%.*]], i64 1
; CHECK-NEXT: [[ARRAYIDX2:%.*]] = getelementptr inbounds i8, i8* [[A]], i64 10
; CHECK-NEXT: [[CMP:%.*]] = icmp slt i8* [[ARRAYIDX1]], [[ARRAYIDX2]]
; CHECK-NEXT: ret i1 [[CMP]]
;
%arrayidx1 = getelementptr inbounds i8, i8* %a, i64 1
%arrayidx2 = getelementptr inbounds i8, i8* %a, i64 10
Expand Down

0 comments on commit e3cec17

Please sign in to comment.