Skip to content
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

[ValueTracking] isNonEqual Pointers with with a recursive GEP #70459

Merged
merged 2 commits into from
Dec 15, 2023

Conversation

bipmis
Copy link
Contributor

@bipmis bipmis commented Oct 27, 2023

Handles canonical icmp eq(ptr1, ptr2) -> where ptr1/ptr2 is a recursive GEP.
Can helps scenarios where InstCombineCompares folds icmp eq(sub(ptr2int, ptr2int), 0) -> icmp eq(ptr1, ptr2)
and
icmp eq(phi(sub(ptr2int, ptr2int), ...)) -> phi i1 (icmp eq(sub(ptr2int, ptr2int), 0), ....)

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 27, 2023

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: None (bipmis)

Changes

Handles canonical icmp eq(ptr1, ptr2) -> where ptr1/ptr2 is a recursive GEP.
Can helps scenarios where InstCombineCompares folds icmp eq(sub(ptr2int, ptr2int), 0) -> icmp eq(ptr1, ptr2)
and
icmp eq(phi(sub(ptr2int, ptr2int), ...)) -> phi i1 (icmp eq(sub(ptr2int, ptr2int), 0), ....)


Patch is 22.45 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/70459.diff

3 Files Affected:

  • (modified) llvm/lib/Analysis/ValueTracking.cpp (+70)
  • (modified) llvm/test/Analysis/ValueTracking/known-non-equal.ll (+37)
  • (modified) llvm/test/Analysis/ValueTracking/phi-known-bits.ll (+370)
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index c303d261107eb19..1fd4ce496fcae9f 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -3141,6 +3141,72 @@ static bool isNonEqualSelect(const Value *V1, const Value *V2, unsigned Depth,
          isKnownNonEqual(SI1->getFalseValue(), V2, Depth + 1, Q);
 }
 
+static bool isNonEqualPointersWithRecursiveGEP(const Value *A, const Value *B,
+                                               unsigned Depth,
+                                               const SimplifyQuery &Q) {
+  // Where A is a recursive GEP for an incoming value of PHI indicating a
+  // loop. B can be a ptr/GEP.
+  // If the PHI has 2 incoming values, one of it being the recursive GEP A
+  // and other a ptr at same base and at an same/higher offset than B we are
+  // only incrementing the pointer further in loop if offset of recursive GEP is
+  // greater than 0.
+  if (!A->getType()->isPointerTy() || !B->getType()->isPointerTy())
+    return false;
+
+  auto *GEPA = dyn_cast<GEPOperator>(A);
+  // Make sure GEPA pointer operand is a PHI.
+  if (!GEPA || !isa<PHINode>(GEPA->getPointerOperand()))
+    return false;
+
+  // Handle 2 incoming PHI values with one being a recursive GEP.
+  auto *PN = dyn_cast<PHINode>(GEPA->getPointerOperand());
+  if (PN->getNumIncomingValues() == 2) {
+    // Recursive GEP in second incoming value. Always keep Recursive GEP as
+    // FirstInst
+    Value *FirstInst = nullptr, *SecondInst = nullptr;
+    for (unsigned i = 0; i != 2; ++i) {
+      // Check if A is a Recursive GEP in one of the incoming values.
+      if (PN->getIncomingValue(i) == A && GEPA->getNumIndices() == 1 &&
+          isa<Constant>(GEPA->idx_begin())) {
+        FirstInst = PN->getIncomingValue(i);
+        SecondInst = PN->getIncomingValue(1 - i);
+        continue;
+      }
+    }
+    if (FirstInst && SecondInst) {
+      // Other incoming node base should match the B base.
+      // SecondInstOffset >= OffsetB && FirstInstOffset > 0?
+      // SecondInstOffset <= OffsetB && FirstInstOffset < 0?
+      // Is non-equal if above are true.
+      APInt FirstInstOffset(Q.DL.getIndexTypeSizeInBits(FirstInst->getType()),
+                            0);
+      FirstInst = FirstInst->stripAndAccumulateConstantOffsets(
+          Q.DL, FirstInstOffset, /* AllowNonInbounds */ true);
+      APInt SecondInstOffset(Q.DL.getIndexTypeSizeInBits(SecondInst->getType()),
+                             0);
+      SecondInst = SecondInst->stripAndAccumulateConstantOffsets(
+          Q.DL, SecondInstOffset, /* AllowNonInbounds */ true);
+      APInt OffsetB(Q.DL.getIndexTypeSizeInBits(B->getType()), 0);
+      B = B->stripAndAccumulateConstantOffsets(Q.DL, OffsetB,
+                                               /* AllowNonInbounds */ true);
+      if (SecondInst == B &&
+          ((SecondInstOffset.sge(OffsetB) && FirstInstOffset.sgt(0)) ||
+           (SecondInstOffset.sle(OffsetB) && FirstInstOffset.slt(0))))
+        return true;
+    }
+  }
+  return false;
+}
+
+static bool isNonEqualPointers(const Value *V1, const Value *V2, unsigned Depth,
+                               const SimplifyQuery &Q) {
+  // Check if A is a recursive GEP which is not equal to B.
+  if (isNonEqualPointersWithRecursiveGEP(V1, V2, Depth, Q))
+    return true;
+
+  return false;
+}
+
 /// Return true if it is known that V1 != V2.
 static bool isKnownNonEqual(const Value *V1, const Value *V2, unsigned Depth,
                             const SimplifyQuery &Q) {
@@ -3194,6 +3260,10 @@ static bool isKnownNonEqual(const Value *V1, const Value *V2, unsigned Depth,
   if (isNonEqualSelect(V1, V2, Depth, Q) || isNonEqualSelect(V2, V1, Depth, Q))
     return true;
 
+  if (isNonEqualPointers(V1, V2, Depth, Q) ||
+      isNonEqualPointers(V2, V1, Depth, Q))
+    return true;
+
   return false;
 }
 
diff --git a/llvm/test/Analysis/ValueTracking/known-non-equal.ll b/llvm/test/Analysis/ValueTracking/known-non-equal.ll
index 79b9b3fd8511b60..365a0ea9bc21dbb 100644
--- a/llvm/test/Analysis/ValueTracking/known-non-equal.ll
+++ b/llvm/test/Analysis/ValueTracking/known-non-equal.ll
@@ -1813,5 +1813,42 @@ exit:
   ret i1 %res
 }
 
+; Illustrate if 2 pointers are non-equal when one of them is a recursive GEP.
+define i1 @icmp_recursiveGEP_withPtr(ptr noundef %val1) {
+; CHECK-LABEL: @icmp_recursiveGEP_withPtr(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP_I:%.*]] = icmp eq ptr [[VAL1:%.*]], null
+; CHECK-NEXT:    br i1 [[CMP_I]], label [[_Z9STRINGLENPKS_EXIT:%.*]], label [[WHILE_COND_I:%.*]]
+; CHECK:       while.cond.i:
+; CHECK-NEXT:    [[A_PN_I:%.*]] = phi ptr [ [[TEST_0_I:%.*]], [[WHILE_COND_I]] ], [ [[VAL1]], [[ENTRY:%.*]] ]
+; CHECK-NEXT:    [[TEST_0_I]] = getelementptr inbounds i8, ptr [[A_PN_I]], i64 1
+; CHECK-NEXT:    [[TMP0:%.*]] = load i8, ptr [[TEST_0_I]], align 2
+; CHECK-NEXT:    [[CMP3_NOT_I:%.*]] = icmp eq i8 [[TMP0]], 0
+; CHECK-NEXT:    br i1 [[CMP3_NOT_I]], label [[WHILE_END_I:%.*]], label [[WHILE_COND_I]]
+; CHECK:       while.end.i:
+; CHECK-NEXT:    br label [[_Z9STRINGLENPKS_EXIT]]
+; CHECK:       _Z9stringlenPKs.exit:
+; CHECK-NEXT:    [[RETVAL_0_I:%.*]] = phi i1 [ false, [[WHILE_END_I]] ], [ true, [[ENTRY]] ]
+; CHECK-NEXT:    ret i1 [[RETVAL_0_I]]
+;
+entry:
+  %cmp.i = icmp eq ptr %val1, null
+  br i1 %cmp.i, label %_Z9stringlenPKs.exit, label %while.cond.i
+
+while.cond.i:
+  %a.pn.i = phi ptr [ %test.0.i, %while.cond.i ], [ %val1, %entry ]
+  %test.0.i = getelementptr inbounds i8, ptr %a.pn.i, i64 1
+  %0 = load i8, ptr %test.0.i, align 2
+  %cmp3.not.i = icmp eq i8 %0, 0
+  br i1 %cmp3.not.i, label %while.end.i, label %while.cond.i
+
+while.end.i:
+  %bool = icmp eq ptr %test.0.i, %val1
+  br label %_Z9stringlenPKs.exit
+
+_Z9stringlenPKs.exit:
+  %retval.0.i = phi i1 [ %bool, %while.end.i ], [ true, %entry ]
+  ret i1 %retval.0.i
+}
 
 !0 = !{ i8 1, i8 5 }
diff --git a/llvm/test/Analysis/ValueTracking/phi-known-bits.ll b/llvm/test/Analysis/ValueTracking/phi-known-bits.ll
index 5227661c837347e..1fed11792f13521 100644
--- a/llvm/test/Analysis/ValueTracking/phi-known-bits.ll
+++ b/llvm/test/Analysis/ValueTracking/phi-known-bits.ll
@@ -424,3 +424,373 @@ T:
 F:
   br label %T
 }
+
+;Illustrate if 2 pointers are non-equal when one of them is a recursive GEP.
+;Cases which folds to a canonical icmp(ptr1, ptr2)
+define i1 @recursiveGEP_withPtr_phisub1(ptr noundef %val1) {
+; CHECK-LABEL: @recursiveGEP_withPtr_phisub1(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP_I:%.*]] = icmp eq ptr [[VAL1:%.*]], null
+; CHECK-NEXT:    br i1 [[CMP_I]], label [[_Z9STRINGLENPKS_EXIT:%.*]], label [[WHILE_COND_I:%.*]]
+; CHECK:       while.cond.i:
+; CHECK-NEXT:    [[A_PN_I:%.*]] = phi ptr [ [[TEST_0_I:%.*]], [[WHILE_COND_I]] ], [ [[VAL1]], [[ENTRY:%.*]] ]
+; CHECK-NEXT:    [[TEST_0_I]] = getelementptr inbounds i8, ptr [[A_PN_I]], i64 1
+; CHECK-NEXT:    [[TMP0:%.*]] = load i8, ptr [[TEST_0_I]], align 2
+; CHECK-NEXT:    [[CMP3_NOT_I:%.*]] = icmp eq i8 [[TMP0]], 0
+; CHECK-NEXT:    br i1 [[CMP3_NOT_I]], label [[WHILE_END_I:%.*]], label [[WHILE_COND_I]]
+; CHECK:       while.end.i:
+; CHECK-NEXT:    br label [[_Z9STRINGLENPKS_EXIT]]
+; CHECK:       _Z9stringlenPKs.exit:
+; CHECK-NEXT:    ret i1 [[CMP_I]]
+;
+entry:
+  %cmp.i = icmp eq ptr %val1, null
+  br i1 %cmp.i, label %_Z9stringlenPKs.exit, label %while.cond.i
+
+while.cond.i:
+  %a.pn.i = phi ptr [ %test.0.i, %while.cond.i ], [ %val1, %entry ]
+  %test.0.i = getelementptr inbounds i8, ptr %a.pn.i, i64 1
+  %0 = load i8, ptr %test.0.i, align 2
+  %cmp3.not.i = icmp eq i8 %0, 0
+  br i1 %cmp3.not.i, label %while.end.i, label %while.cond.i
+
+while.end.i:
+  %sub.ptr.lhs.cast.i = ptrtoint ptr %test.0.i to i64
+  %sub.ptr.rhs.cast.i = ptrtoint ptr %val1 to i64
+  %sub.ptr.sub.i = sub i64 %sub.ptr.lhs.cast.i, %sub.ptr.rhs.cast.i
+  br label %_Z9stringlenPKs.exit
+
+_Z9stringlenPKs.exit:
+  %retval.0.i = phi i64 [ %sub.ptr.sub.i, %while.end.i ], [ 0, %entry ]
+  %bool = icmp eq i64 %retval.0.i, 0
+  ret i1 %bool
+}
+
+define i1 @recursiveGEP_withPtr_phisub1_PhiOperandsCommuted(ptr noundef %val1) {
+; CHECK-LABEL: @recursiveGEP_withPtr_phisub1_PhiOperandsCommuted(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP_I:%.*]] = icmp eq ptr [[VAL1:%.*]], null
+; CHECK-NEXT:    br i1 [[CMP_I]], label [[_Z9STRINGLENPKS_EXIT:%.*]], label [[WHILE_COND_I:%.*]]
+; CHECK:       while.cond.i:
+; CHECK-NEXT:    [[A_PN_I:%.*]] = phi ptr [ [[VAL1]], [[ENTRY:%.*]] ], [ [[TEST_0_I:%.*]], [[WHILE_COND_I]] ]
+; CHECK-NEXT:    [[TEST_0_I]] = getelementptr inbounds i8, ptr [[A_PN_I]], i64 1
+; CHECK-NEXT:    [[TMP0:%.*]] = load i8, ptr [[TEST_0_I]], align 2
+; CHECK-NEXT:    [[CMP3_NOT_I:%.*]] = icmp eq i8 [[TMP0]], 0
+; CHECK-NEXT:    br i1 [[CMP3_NOT_I]], label [[WHILE_END_I:%.*]], label [[WHILE_COND_I]]
+; CHECK:       while.end.i:
+; CHECK-NEXT:    br label [[_Z9STRINGLENPKS_EXIT]]
+; CHECK:       _Z9stringlenPKs.exit:
+; CHECK-NEXT:    ret i1 [[CMP_I]]
+;
+entry:
+  %cmp.i = icmp eq ptr %val1, null
+  br i1 %cmp.i, label %_Z9stringlenPKs.exit, label %while.cond.i
+
+while.cond.i:
+  %a.pn.i = phi ptr [ %val1, %entry ], [ %test.0.i, %while.cond.i ]
+  %test.0.i = getelementptr inbounds i8, ptr %a.pn.i, i64 1
+  %0 = load i8, ptr %test.0.i, align 2
+  %cmp3.not.i = icmp eq i8 %0, 0
+  br i1 %cmp3.not.i, label %while.end.i, label %while.cond.i
+
+while.end.i:
+  %sub.ptr.lhs.cast.i = ptrtoint ptr %test.0.i to i64
+  %sub.ptr.rhs.cast.i = ptrtoint ptr %val1 to i64
+  %sub.ptr.sub.i = sub i64 %sub.ptr.lhs.cast.i, %sub.ptr.rhs.cast.i
+  br label %_Z9stringlenPKs.exit
+
+_Z9stringlenPKs.exit:
+  %retval.0.i = phi i64 [ %sub.ptr.sub.i, %while.end.i ], [ 0, %entry ]
+  %bool = icmp eq i64 %retval.0.i, 0
+  ret i1 %bool
+}
+
+define i1 @recursiveGEP_withPtr_phisub1_SubOperandsCommuted(ptr noundef %val1) {
+; CHECK-LABEL: @recursiveGEP_withPtr_phisub1_SubOperandsCommuted(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP_I:%.*]] = icmp eq ptr [[VAL1:%.*]], null
+; CHECK-NEXT:    br i1 [[CMP_I]], label [[_Z9STRINGLENPKS_EXIT:%.*]], label [[WHILE_COND_I:%.*]]
+; CHECK:       while.cond.i:
+; CHECK-NEXT:    [[A_PN_I:%.*]] = phi ptr [ [[TEST_0_I:%.*]], [[WHILE_COND_I]] ], [ [[VAL1]], [[ENTRY:%.*]] ]
+; CHECK-NEXT:    [[TEST_0_I]] = getelementptr inbounds i8, ptr [[A_PN_I]], i64 1
+; CHECK-NEXT:    [[TMP0:%.*]] = load i8, ptr [[TEST_0_I]], align 2
+; CHECK-NEXT:    [[CMP3_NOT_I:%.*]] = icmp eq i8 [[TMP0]], 0
+; CHECK-NEXT:    br i1 [[CMP3_NOT_I]], label [[WHILE_END_I:%.*]], label [[WHILE_COND_I]]
+; CHECK:       while.end.i:
+; CHECK-NEXT:    br label [[_Z9STRINGLENPKS_EXIT]]
+; CHECK:       _Z9stringlenPKs.exit:
+; CHECK-NEXT:    ret i1 [[CMP_I]]
+;
+entry:
+  %cmp.i = icmp eq ptr %val1, null
+  br i1 %cmp.i, label %_Z9stringlenPKs.exit, label %while.cond.i
+
+while.cond.i:
+  %a.pn.i = phi ptr [ %test.0.i, %while.cond.i ], [ %val1, %entry ]
+  %test.0.i = getelementptr inbounds i8, ptr %a.pn.i, i64 1
+  %0 = load i8, ptr %test.0.i, align 2
+  %cmp3.not.i = icmp eq i8 %0, 0
+  br i1 %cmp3.not.i, label %while.end.i, label %while.cond.i
+
+while.end.i:
+  %sub.ptr.lhs.cast.i = ptrtoint ptr %test.0.i to i64
+  %sub.ptr.rhs.cast.i = ptrtoint ptr %val1 to i64
+  %sub.ptr.sub.i = sub i64 %sub.ptr.rhs.cast.i, %sub.ptr.lhs.cast.i
+  br label %_Z9stringlenPKs.exit
+
+_Z9stringlenPKs.exit:
+  %retval.0.i = phi i64 [ %sub.ptr.sub.i, %while.end.i ], [ 0, %entry ]
+  %bool = icmp eq i64 %retval.0.i, 0
+  ret i1 %bool
+}
+
+define i1 @recursiveGEP_withPtr_phisub2(ptr noundef %val1) {
+; CHECK-LABEL: @recursiveGEP_withPtr_phisub2(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP_I:%.*]] = icmp eq ptr [[VAL1:%.*]], null
+; CHECK-NEXT:    br i1 [[CMP_I]], label [[_Z9STRINGLENPKS_EXIT:%.*]], label [[WHILE_COND_I:%.*]]
+; CHECK:       while.cond.i:
+; CHECK-NEXT:    [[A_PN_I:%.*]] = phi ptr [ [[TEST_0_I:%.*]], [[WHILE_COND_I]] ], [ [[VAL1]], [[ENTRY:%.*]] ]
+; CHECK-NEXT:    [[TEST_0_I]] = getelementptr inbounds i8, ptr [[A_PN_I]], i64 -1
+; CHECK-NEXT:    [[TMP0:%.*]] = load i8, ptr [[TEST_0_I]], align 2
+; CHECK-NEXT:    [[CMP3_NOT_I:%.*]] = icmp eq i8 [[TMP0]], 0
+; CHECK-NEXT:    br i1 [[CMP3_NOT_I]], label [[WHILE_END_I:%.*]], label [[WHILE_COND_I]]
+; CHECK:       while.end.i:
+; CHECK-NEXT:    br label [[_Z9STRINGLENPKS_EXIT]]
+; CHECK:       _Z9stringlenPKs.exit:
+; CHECK-NEXT:    ret i1 [[CMP_I]]
+;
+entry:
+  %cmp.i = icmp eq ptr %val1, null
+  br i1 %cmp.i, label %_Z9stringlenPKs.exit, label %while.cond.i
+
+while.cond.i:
+  %a.pn.i = phi ptr [ %test.0.i, %while.cond.i ], [ %val1, %entry ]
+  %test.0.i = getelementptr inbounds i8, ptr %a.pn.i, i64 -1
+  %0 = load i8, ptr %test.0.i, align 2
+  %cmp3.not.i = icmp eq i8 %0, 0
+  br i1 %cmp3.not.i, label %while.end.i, label %while.cond.i
+
+while.end.i:
+  %sub.ptr.lhs.cast.i = ptrtoint ptr %test.0.i to i64
+  %sub.ptr.rhs.cast.i = ptrtoint ptr %val1 to i64
+  %sub.ptr.sub.i = sub i64 %sub.ptr.lhs.cast.i, %sub.ptr.rhs.cast.i
+  br label %_Z9stringlenPKs.exit
+
+_Z9stringlenPKs.exit:
+  %retval.0.i = phi i64 [ %sub.ptr.sub.i, %while.end.i ], [ 0, %entry ]
+  %bool = icmp eq i64 %retval.0.i, 0
+  ret i1 %bool
+}
+
+define i1 @recursiveGEP_withPtr_phisub3(ptr noundef %val1) {
+; CHECK-LABEL: @recursiveGEP_withPtr_phisub3(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TEST_VAL1:%.*]] = getelementptr inbounds i8, ptr [[VAL1:%.*]], i64 7
+; CHECK-NEXT:    [[CMP_I:%.*]] = icmp eq ptr [[VAL1]], null
+; CHECK-NEXT:    br i1 [[CMP_I]], label [[_Z9STRINGLENPKS_EXIT:%.*]], label [[WHILE_COND_I:%.*]]
+; CHECK:       while.cond.i:
+; CHECK-NEXT:    [[A_PN_I:%.*]] = phi ptr [ [[TEST_0_I:%.*]], [[WHILE_COND_I]] ], [ [[TEST_VAL1]], [[ENTRY:%.*]] ]
+; CHECK-NEXT:    [[TEST_0_I]] = getelementptr inbounds i8, ptr [[A_PN_I]], i64 1
+; CHECK-NEXT:    [[TMP0:%.*]] = load i8, ptr [[TEST_0_I]], align 2
+; CHECK-NEXT:    [[CMP3_NOT_I:%.*]] = icmp eq i8 [[TMP0]], 0
+; CHECK-NEXT:    br i1 [[CMP3_NOT_I]], label [[WHILE_END_I:%.*]], label [[WHILE_COND_I]]
+; CHECK:       while.end.i:
+; CHECK-NEXT:    br label [[_Z9STRINGLENPKS_EXIT]]
+; CHECK:       _Z9stringlenPKs.exit:
+; CHECK-NEXT:    ret i1 [[CMP_I]]
+;
+entry:
+  %test.val1 = getelementptr inbounds i8, ptr %val1, i64 7
+  %cmp.i = icmp eq ptr %val1, null
+  br i1 %cmp.i, label %_Z9stringlenPKs.exit, label %while.cond.i
+
+while.cond.i:
+  %a.pn.i = phi ptr [ %test.0.i, %while.cond.i ], [ %test.val1, %entry ]
+  %test.0.i = getelementptr inbounds i8, ptr %a.pn.i, i64 1
+  %0 = load i8, ptr %test.0.i, align 2
+  %cmp3.not.i = icmp eq i8 %0, 0
+  br i1 %cmp3.not.i, label %while.end.i, label %while.cond.i
+
+while.end.i:
+  %sub.ptr.lhs.cast.i = ptrtoint ptr %test.0.i to i64
+  %1 = getelementptr inbounds i8, ptr %val1, i64 5
+  %sub.ptr.rhs.cast.i = ptrtoint ptr %1 to i64
+  %sub.ptr.sub.i = sub i64 %sub.ptr.lhs.cast.i, %sub.ptr.rhs.cast.i
+  br label %_Z9stringlenPKs.exit
+
+_Z9stringlenPKs.exit:
+  %retval.0.i = phi i64 [ %sub.ptr.sub.i, %while.end.i ], [ 0, %entry ]
+  %bool = icmp eq i64 %retval.0.i, 0
+  ret i1 %bool
+}
+
+define i1 @recursiveGEP_withPtr_phisub1_notKnownNonEqual1(ptr noundef %val1, i64 %val2) {
+; CHECK-LABEL: @recursiveGEP_withPtr_phisub1_notKnownNonEqual1(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP_I:%.*]] = icmp eq ptr [[VAL1:%.*]], null
+; CHECK-NEXT:    br i1 [[CMP_I]], label [[_Z9STRINGLENPKS_EXIT:%.*]], label [[WHILE_COND_I:%.*]]
+; CHECK:       while.cond.i:
+; CHECK-NEXT:    [[A_PN_I:%.*]] = phi ptr [ [[TEST_0_I:%.*]], [[WHILE_COND_I]] ], [ [[VAL1]], [[ENTRY:%.*]] ]
+; CHECK-NEXT:    [[TEST_0_I]] = getelementptr inbounds i8, ptr [[A_PN_I]], i64 1
+; CHECK-NEXT:    [[TMP0:%.*]] = load i8, ptr [[TEST_0_I]], align 2
+; CHECK-NEXT:    [[CMP3_NOT_I:%.*]] = icmp eq i8 [[TMP0]], 0
+; CHECK-NEXT:    br i1 [[CMP3_NOT_I]], label [[WHILE_END_I:%.*]], label [[WHILE_COND_I]]
+; CHECK:       while.end.i:
+; CHECK-NEXT:    [[TMP1:%.*]] = getelementptr inbounds i8, ptr [[VAL1]], i64 [[VAL2:%.*]]
+; CHECK-NEXT:    [[TMP2:%.*]] = icmp eq ptr [[TEST_0_I]], [[TMP1]]
+; CHECK-NEXT:    br label [[_Z9STRINGLENPKS_EXIT]]
+; CHECK:       _Z9stringlenPKs.exit:
+; CHECK-NEXT:    [[RETVAL_0_I:%.*]] = phi i1 [ [[TMP2]], [[WHILE_END_I]] ], [ true, [[ENTRY]] ]
+; CHECK-NEXT:    ret i1 [[RETVAL_0_I]]
+;
+entry:
+  %cmp.i = icmp eq ptr %val1, null
+  br i1 %cmp.i, label %_Z9stringlenPKs.exit, label %while.cond.i
+
+while.cond.i:
+  %a.pn.i = phi ptr [ %test.0.i, %while.cond.i ], [ %val1, %entry ]
+  %test.0.i = getelementptr inbounds i8, ptr %a.pn.i, i64 1
+  %0 = load i8, ptr %test.0.i, align 2
+  %cmp3.not.i = icmp eq i8 %0, 0
+  br i1 %cmp3.not.i, label %while.end.i, label %while.cond.i
+
+while.end.i:
+  %sub.ptr.lhs.cast.i = ptrtoint ptr %test.0.i to i64
+  %1 = getelementptr inbounds i8, ptr %val1, i64 %val2
+  %sub.ptr.rhs.cast.i = ptrtoint ptr %1 to i64
+  %sub.ptr.sub.i = sub i64 %sub.ptr.lhs.cast.i, %sub.ptr.rhs.cast.i
+  br label %_Z9stringlenPKs.exit
+
+_Z9stringlenPKs.exit:
+  %retval.0.i = phi i64 [ %sub.ptr.sub.i, %while.end.i ], [ 0, %entry ]
+  %bool = icmp eq i64 %retval.0.i, 0
+  ret i1 %bool
+}
+
+define i1 @recursiveGEP_withPtr_phisub1_notKnownNonEqual2(ptr noundef %val1) {
+; CHECK-LABEL: @recursiveGEP_withPtr_phisub1_notKnownNonEqual2(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TEST_VAL1:%.*]] = getelementptr inbounds i8, ptr [[VAL1:%.*]], i64 -1
+; CHECK-NEXT:    [[CMP_I:%.*]] = icmp eq ptr [[VAL1]], null
+; CHECK-NEXT:    br i1 [[CMP_I]], label [[_Z9STRINGLENPKS_EXIT:%.*]], label [[WHILE_COND_I:%.*]]
+; CHECK:       while.cond.i:
+; CHECK-NEXT:    [[A_PN_I:%.*]] = phi ptr [ [[TEST_0_I:%.*]], [[WHILE_COND_I]] ], [ [[TEST_VAL1]], [[ENTRY:%.*]] ]
+; CHECK-NEXT:    [[TEST_0_I]] = getelementptr inbounds i8, ptr [[A_PN_I]], i64 1
+; CHECK-NEXT:    [[TMP0:%.*]] = load i8, ptr [[TEST_0_I]], align 2
+; CHECK-NEXT:    [[CMP3_NOT_I:%.*]] = icmp eq i8 [[TMP0]], 0
+; CHECK-NEXT:    br i1 [[CMP3_NOT_I]], label [[WHILE_END_I:%.*]], label [[WHILE_COND_I]]
+; CHECK:       while.end.i:
+; CHECK-NEXT:    [[TMP1:%.*]] = icmp eq ptr [[TEST_0_I]], [[VAL1]]
+; CHECK-NEXT:    br label [[_Z9STRINGLENPKS_EXIT]]
+; CHECK:       _Z9stringlenPKs.exit:
+; CHECK-NEXT:    [[RETVAL_0_I:%.*]] = phi i1 [ [[TMP1]], [[WHILE_END_I]] ], [ true, [[ENTRY]] ]
+; CHECK-NEXT:    ret i1 [[RETVAL_0_I]]
+;
+entry:
+  %test.val1 = getelementptr inbounds i8, ptr %val1, i64 -1
+  %cmp.i = icmp eq ptr %val1, null
+  br i1 %cmp.i, label %_Z9stringlenPKs.exit, label %while.cond.i
+
+while.cond.i:
+  %a.pn.i = phi ptr [ %test.0.i, %while.cond.i ], [ %test.val1, %entry ]
+  %test.0.i = getelementptr inbounds i8, ptr %a.pn.i, i64 1
+  %0 = load i8, ptr %test.0.i, align 2
+  %cmp3.not.i = icmp eq i8 %0, 0
+  br i1 %cmp3.not.i, label %while.end.i, label %while.cond.i
+
+while.end.i:
+  %sub.ptr.lhs.cast.i = ptrtoint ptr %test.0.i to i64
+  %sub.ptr.rhs.cast.i = ptrtoint ptr %val1 to i64
+  %sub.ptr.sub.i = sub i64 %sub.ptr.lhs.cast.i, %sub.ptr.rhs.cast.i
+  br label %_Z9stringlenPKs.exit
+
+_Z9stringlenPKs.exit:
+  %retval.0.i = phi i64 [ %sub.ptr.sub.i, %while.end.i ], [ 0, %entry ]
+  %bool = icmp eq i64 %retval.0.i, 0
+  ret i1 %bool
+}
+
+define i1 @recursiveGEP_withPtr_phisub1_notKnownNonEqual3(ptr noundef %val1) {
+; CHECK-LABEL: @recursiveGEP_withPtr_phisub1_notKnownNonEqual3(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TEST_VAL1:%.*]] = getelementptr inbounds i8, ptr [[VAL1:%.*]], i64 5
+; CHECK-NEXT:    [[CMP_I:%.*]] = icmp eq ptr [[VAL1]], null
+; CHECK-NEXT:    br i1 [[CMP_I]], label [[_Z9STRINGLENPKS_EXIT:%.*]], label [[WHILE_COND_I:%.*]]
+; CHECK:       while.cond.i:
+; CHECK-NEXT:    [[A_PN_I:%.*]] = phi ptr [ [[TEST_0_I:%.*]], [[WHILE_COND_I]] ], [ [[TEST_VAL1]], [[ENTRY:%.*]] ]
+; CHECK-NEXT:    [[TEST_0_I]] = getelementptr inbounds i8, ptr [[A_PN_I]], i64 -1
+; CHECK-NEXT:    [[TMP0:%.*]] = load i8, ptr [[TEST_0_I]], align 2
+; CHECK-NEXT:    [[CMP3_NOT_I:%.*]] = icmp eq i8 [[TMP0]], 0
+; CHECK-NEXT:    br i1 [[CMP3_NOT_I]], label [[WHILE_END_I:%.*]], label [[WHILE_COND_I]]
+; CHECK:       while.end.i:
+; CHECK-NEXT:    [[TMP1:%.*]] = icmp eq ptr [[TEST_0_I]], [[VAL1]]
+; CHECK-NEXT:    br label [[_Z9STRINGLENPKS_EXIT]]
+; CHECK:       _Z9stringlenPKs.exit:
+; CHECK-NEXT:    [[RETVAL_0_I:%.*]] = phi i1 [ [[TMP1]], [[WHILE_END_I]] ], [ true, [[ENTRY]] ]
+; CHECK-NEXT:    ret i1 [[RETVAL_0_I]]
+;
+entry:
+  %test.val1 = getelementptr inbounds i8, ptr %val1, i64 5
+  %cmp.i = icmp eq ptr %val1, null
+  br i1 %cmp.i, label %_Z9stringlenPKs.exit, label %while.cond.i
+
+while.cond...
[truncated]

@bipmis
Copy link
Contributor Author

bipmis commented Nov 2, 2023

Ping

llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
@bipmis
Copy link
Contributor Author

bipmis commented Nov 24, 2023

Ping. Let me know if there are any other review comments. Thanks.

@bipmis
Copy link
Contributor Author

bipmis commented Dec 6, 2023

Ping

llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
ret i1 %bool
}

define i1 @recursiveGEP_withPtrSub_noninbound2(ptr noundef %val1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a TODO here for potential future work? In theory even without the inbounds flag we know that %test.0.i can never equal %val1 because we add 0x100000001 to %test.0.i in each iteration. Even if it wraps in the address space the closest it gets to %val1 is %val1 + 1, hence %sub.ptr.sub.i will never be 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the point of view I had as well. If the base pointer matches, anything that suggests an increment/decrement(irrespective of inbounds flag) should still be inequal.
@nikic I took from your suggestion that it is better to handle inbound cases for now. But if there is anything else to it do suggest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only issue here could be the need to handle exactly the scenario where index is a value(positive/negative) equivalent to 0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For non-inbounds GEPs, whether it can be equal depends on the precise relation of the start, step and limit. You basically have the question whether Step * N == Limit - Start over the ring Z/(2^BW) has a solution. This question does have a fairly simple answer, but I don't think you should implement handling for this without cause.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I have added more test to showcase that this is being handled. Only exception to this is when exactly the same pointer is used for Start and Limit. Alternatively this can be handled by early check on Start/Limit being a non-inbound GEP.

llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
// Check if A is a Recursive GEP in one of the incoming values.
if (PN->getIncomingValue(i) == A && GEPA->getNumIndices() == 1 &&
isa<Constant>(GEPA->idx_begin())) {
Step = PN->getIncomingValue(i);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the word Step here is a bit misleading, because it implies it's the difference between two values. However, it's really the updated ptr value for the next iteration. How about NextStep or NextVal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree however this seems to be commonly used for these recurring loops. Have kept it the same for now.

llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
@bipmis bipmis force-pushed the ValueTrackingIsNonEqualPointers branch from a024401 to ffdae34 Compare December 12, 2023 14:22
@bipmis
Copy link
Contributor Author

bipmis commented Dec 12, 2023

Have added tests to show more non-inbounds scenario. If there are any further comments please suggest. Thanks.

Copy link
Contributor

@david-arm david-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks almost ready to go I think! I just have some more minor comments ...

llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
ret i1 %bool
}

define i1 @recursiveGEP_withPtrSub_noninbound_StepLimit(ptr noundef %val1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what StepLimit means here to be honest. Also, I'm not sure what value this adds over @recursiveGEP_withPtrSub_noninbound_Step1 given both are non-inbounds GEPs. Is the idea that in future you think we can add support for one of these non-inbounds GEP cases? If so, perhaps worth adding comments on the tests you think we could support.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is to indicate which ones in the test are non-inbounds. Have added necessary comments.

llvm/test/Analysis/ValueTracking/phi-known-bits.ll Outdated Show resolved Hide resolved
llvm/test/Analysis/ValueTracking/phi-known-bits.ll Outdated Show resolved Hide resolved
@bipmis bipmis force-pushed the ValueTrackingIsNonEqualPointers branch 2 times, most recently from 38d5936 to fd8996e Compare December 12, 2023 18:21
Copy link
Contributor

@david-arm david-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for all the changes @bipmis. :)

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code is correct now, just some style nits.

llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
llvm/lib/Analysis/ValueTracking.cpp Outdated Show resolved Hide resolved
@bipmis bipmis force-pushed the ValueTrackingIsNonEqualPointers branch from fd8996e to acc493f Compare December 13, 2023 12:41
@bipmis
Copy link
Contributor Author

bipmis commented Dec 13, 2023

I think the code is correct now, just some style nits.

Handled the same. Thanks

@bipmis bipmis force-pushed the ValueTrackingIsNonEqualPointers branch from acc493f to d31cb1b Compare December 13, 2023 12:49
@bipmis bipmis force-pushed the ValueTrackingIsNonEqualPointers branch from d31cb1b to e5e3884 Compare December 13, 2023 14:49
Comment on lines 3146 to 3148
return (Start == B &&
((StartOffset.sge(OffsetB) && StepOffset.isStrictlyPositive()) ||
(StartOffset.sle(OffsetB) && StepOffset.isNegative())));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return (Start == B &&
((StartOffset.sge(OffsetB) && StepOffset.isStrictlyPositive()) ||
(StartOffset.sle(OffsetB) && StepOffset.isNegative())));
return Start == B &&
((StartOffset.sge(OffsetB) && StepOffset.isStrictlyPositive()) ||
(StartOffset.sle(OffsetB) && StepOffset.isNegative()));


;Illustrate if 2 pointers are non-equal when one of them is a recursive GEP.
;Cases which folds to a canonical icmp(ptr1, ptr2)
define i1 @recursiveGEP_withPtrSub1(ptr noundef %val1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop all the noundefs (assuming they're not relevant).

@bipmis bipmis force-pushed the ValueTrackingIsNonEqualPointers branch from e5e3884 to 5557794 Compare December 14, 2023 15:30
@bipmis bipmis force-pushed the ValueTrackingIsNonEqualPointers branch from 5557794 to 5c51935 Compare December 14, 2023 15:38
@bipmis bipmis merged commit 6df6320 into llvm:main Dec 15, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants