From e3cec17b2db292227a2b92d46e653372dad711af Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Sat, 6 Nov 2021 12:54:29 +0100 Subject: [PATCH] [InstSimplify] Remove incorrect icmp of gep fold (PR52429) 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 --- llvm/lib/Analysis/InstructionSimplify.cpp | 24 ---------------------- llvm/test/Transforms/InstCombine/icmp-custom-dl.ll | 11 ++++++++-- llvm/test/Transforms/InstCombine/icmp.ll | 11 ++++++++-- llvm/test/Transforms/InstSimplify/compare.ll | 8 ++++++-- 4 files changed, 24 insertions(+), 30 deletions(-) diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp b/llvm/lib/Analysis/InstructionSimplify.cpp index 6a27dd7..d406583 100644 --- a/llvm/lib/Analysis/InstructionSimplify.cpp +++ b/llvm/lib/Analysis/InstructionSimplify.cpp @@ -3665,30 +3665,6 @@ static Value *SimplifyICmpInst(unsigned Predicate, Value *LHS, Value *RHS, CRHS->getPointerOperand(), Q)) return C; - if (GetElementPtrInst *GLHS = dyn_cast(LHS)) { - if (GEPOperator *GRHS = dyn_cast(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 IndicesLHS(GLHS->indices()); - Constant *NewLHS = ConstantExpr::getGetElementPtr( - GLHS->getSourceElementType(), Null, IndicesLHS); - - SmallVector 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(LHS) || isa(RHS)) diff --git a/llvm/test/Transforms/InstCombine/icmp-custom-dl.ll b/llvm/test/Transforms/InstCombine/icmp-custom-dl.ll index 6e76525..5accd3e 100644 --- a/llvm/test/Transforms/InstCombine/icmp-custom-dl.ll +++ b/llvm/test/Transforms/InstCombine/icmp-custom-dl.ll @@ -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 @@ -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 diff --git a/llvm/test/Transforms/InstCombine/icmp.ll b/llvm/test/Transforms/InstCombine/icmp.ll index 3122743..dc7282c 100644 --- a/llvm/test/Transforms/InstCombine/icmp.ll +++ b/llvm/test/Transforms/InstCombine/icmp.ll @@ -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 @@ -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 diff --git a/llvm/test/Transforms/InstSimplify/compare.ll b/llvm/test/Transforms/InstSimplify/compare.ll index 834b0be..1aebe54 100644 --- a/llvm/test/Transforms/InstSimplify/compare.ll +++ b/llvm/test/Transforms/InstSimplify/compare.ll @@ -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 @@ -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 -- 2.7.4