[llvm] 872d69e - [InstCombine] Fix inbounds preservation when merging GEPs (PR55722)

[Nikita Popov via llvm-commits]

Author: Nikita Popov
Date: 2022-05-31T11:54:01+02:00
New Revision: 872d69e5d4e251bd70c6cf5dbf1b3ea34f976aa2

URL: https://github.com/llvm/llvm-project/commit/872d69e5d4e251bd70c6cf5dbf1b3ea34f976aa2
DIFF: https://github.com/llvm/llvm-project/commit/872d69e5d4e251bd70c6cf5dbf1b3ea34f976aa2.diff

LOG: [InstCombine] Fix inbounds preservation when merging GEPs (PR55722)

Even if the total offset is inbounds, we might represent it by first
performing a large negative offset and then a small positive one.
With inbounds semantics as currently specified, each offset must
be inbounds individually, not just the overall offset of the GEP.

Fix this by checking that the sign of all offsets is the same.

Fixes https://github.com/llvm/llvm-project/issues/55722.

Added: 
    

Modified: 
    llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
    llvm/test/Transforms/InstCombine/gep-merge-constant-indices.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 4e8f3af2b5bef..07f44036cce5a 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -2035,13 +2035,20 @@ Instruction *InstCombinerImpl::visitGEPOfGEP(GetElementPtrInst &GEP,
     if (!Offset.isZero() || (!IsFirstType && !ConstIndices[0].isZero()))
       return nullptr;
 
+    bool IsInBounds = isMergedGEPInBounds(*Src, *cast<GEPOperator>(&GEP));
     SmallVector<Value *> Indices;
     append_range(Indices, drop_end(Src->indices(),
                                    Src->getNumIndices() - NumVarIndices));
-    for (const APInt &Idx : drop_begin(ConstIndices, !IsFirstType))
+    for (const APInt &Idx : drop_begin(ConstIndices, !IsFirstType)) {
       Indices.push_back(ConstantInt::get(GEP.getContext(), Idx));
+      // Even if the total offset is inbounds, we may end up representing it
+      // by first performing a larger negative offset, and then a smaller
+      // positive one. The large negative offset might go out of bounds. Only
+      // preserve inbounds if all signs are the same.
+      IsInBounds &= Idx.isNonNegative() == ConstIndices[0].isNonNegative();
+    }
 
-    return isMergedGEPInBounds(*Src, *cast<GEPOperator>(&GEP))
+    return IsInBounds
                ? GetElementPtrInst::CreateInBounds(Src->getSourceElementType(),
                                                    Src->getOperand(0), Indices,
                                                    GEP.getName())

diff  --git a/llvm/test/Transforms/InstCombine/gep-merge-constant-indices.ll b/llvm/test/Transforms/InstCombine/gep-merge-constant-indices.ll
index af79faf265feb..b82c366ca9e94 100644
--- a/llvm/test/Transforms/InstCombine/gep-merge-constant-indices.ll
+++ b/llvm/test/Transforms/InstCombine/gep-merge-constant-indices.ll
@@ -106,7 +106,7 @@ define ptr @struct1(ptr %p) {
 ; result = &((struct.A*) p - 1).member1
 define ptr @struct2(ptr %p) {
 ; CHECK-LABEL: @struct2(
-; CHECK-NEXT:    [[TMP1:%.*]] = getelementptr inbounds [[STRUCT_A:%.*]], ptr [[P:%.*]], i64 -1, i32 1
+; CHECK-NEXT:    [[TMP1:%.*]] = getelementptr [[STRUCT_A:%.*]], ptr [[P:%.*]], i64 -1, i32 1
 ; CHECK-NEXT:    ret ptr [[TMP1]]
 ;
   %1 = getelementptr inbounds %struct.A, ptr %p, i64 0, i32 1