ValueTracking: generalize isNonEqualPHIs #108820

artagnon · 2024-09-16T12:29:06Z

isNonEqualPHIs was originally added in 3fd64cc ([ValueTracking] Handle two PHIs in isKnownNonEqual()), but as the new test case shows, it is not powerful enough. Generalize it, removing the APInt-specific comparison, and extend isKnownNonEqual to handle the case when one of the operands is a constant.

llvmbot · 2024-09-16T12:29:44Z

@llvm/pr-subscribers-llvm-analysis

Author: Ramkumar Ramachandra (artagnon)

Changes

isNonEqualPHIs was originally added in 3fd64cc ([ValueTracking] Handle two PHIs in isKnownNonEqual()), but as the new test case shows, it is not powerful enough. Generalize it, removing the APInt-specific comparison, and extend isKnownNonEqual to handle the case when one of the operands is a constant.

Full diff: https://github.com/llvm/llvm-project/pull/108820.diff

2 Files Affected:

(modified) llvm/lib/Analysis/ValueTracking.cpp (+6-15)
(modified) llvm/test/Analysis/ValueTracking/known-non-equal.ll (+26)

diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index ba3ba7cc98136d..c46152f60aab32 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -2696,11 +2696,6 @@ static bool isNonZeroSub(const APInt &DemandedElts, unsigned Depth,
   if (matchOpWithOpEqZero(X, Y))
     return true;
 
-  // TODO: Move this case into isKnownNonEqual().
-  if (auto *C = dyn_cast<Constant>(X))
-    if (C->isNullValue() && isKnownNonZero(Y, DemandedElts, Q, Depth))
-      return true;
-
   return ::isKnownNonEqual(X, Y, DemandedElts, Depth, Q);
 }
 
@@ -3536,25 +3531,15 @@ static bool isNonEqualPHIs(const PHINode *PN1, const PHINode *PN2,
     return false;
 
   SmallPtrSet<const BasicBlock *, 8> VisitedBBs;
-  bool UsedFullRecursion = false;
   for (const BasicBlock *IncomBB : PN1->blocks()) {
     if (!VisitedBBs.insert(IncomBB).second)
       continue; // Don't reprocess blocks that we have dealt with already.
     const Value *IV1 = PN1->getIncomingValueForBlock(IncomBB);
     const Value *IV2 = PN2->getIncomingValueForBlock(IncomBB);
-    const APInt *C1, *C2;
-    if (match(IV1, m_APInt(C1)) && match(IV2, m_APInt(C2)) && *C1 != *C2)
-      continue;
-
-    // Only one pair of phi operands is allowed for full recursion.
-    if (UsedFullRecursion)
-      return false;
-
     SimplifyQuery RecQ = Q.getWithoutCondContext();
     RecQ.CxtI = IncomBB->getTerminator();
     if (!isKnownNonEqual(IV1, IV2, DemandedElts, Depth + 1, RecQ))
       return false;
-    UsedFullRecursion = true;
   }
   return true;
 }
@@ -3641,6 +3626,12 @@ static bool isKnownNonEqual(const Value *V1, const Value *V2,
     // We can't look through casts yet.
     return false;
 
+  if (isa<Constant>(V2))
+    std::swap(V1, V2);
+  if (auto *C = dyn_cast<Constant>(V1))
+    if (C->isNullValue() && isKnownNonZero(V2, DemandedElts, Q, Depth))
+      return true;
+
   if (Depth >= MaxAnalysisRecursionDepth)
     return false;
 
diff --git a/llvm/test/Analysis/ValueTracking/known-non-equal.ll b/llvm/test/Analysis/ValueTracking/known-non-equal.ll
index d67f1b56621476..824a62b618d983 100644
--- a/llvm/test/Analysis/ValueTracking/known-non-equal.ll
+++ b/llvm/test/Analysis/ValueTracking/known-non-equal.ll
@@ -316,6 +316,32 @@ exit:
   ret i1 %cmp
 }
 
+define i1 @known_non_equal_phis2(i8 %p, i8 %n) {
+; CHECK-LABEL: @known_non_equal_phis2(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[LOOP:%.*]]
+; CHECK:       loop:
+; CHECK-NEXT:    [[A:%.*]] = phi i8 [ 0, [[ENTRY:%.*]] ], [ [[NEXT:%.*]], [[LOOP]] ]
+; CHECK-NEXT:    [[NEXT]] = add nsw i8 [[A]], 2
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp eq i8 [[A]], [[N:%.*]]
+; CHECK-NEXT:    br i1 [[CMP1]], label [[EXIT:%.*]], label [[LOOP]]
+; CHECK:       exit:
+; CHECK-NEXT:    ret i1 true
+;
+entry:
+  %known.nonzero = add nuw i8 %p, 1
+  br label %loop
+loop:
+  %A = phi i8 [ 0, %entry ], [ %next, %loop ]
+  %B = phi i8 [ %known.nonzero, %entry ], [ %A, %loop ]
+  %next = add nsw i8 %A, 2
+  %cmp1 = icmp eq i8 %A, %n
+  br i1 %cmp1, label %exit, label %loop
+exit:
+  %cmp = icmp ne i8 %A, %B
+  ret i1 %cmp
+}
+
 define i1 @known_non_equal_phis_fail(i8 %p, ptr %pq, i8 %n, i8 %r) {
 ; CHECK-LABEL: @known_non_equal_phis_fail(
 ; CHECK-NEXT:  entry:

nikic

The limitation exists for compile-time reasons. You will get exponential blowup if you run full recursion over all phi inputs.

artagnon · 2024-09-16T13:04:30Z

The limitation exists for compile-time reasons. You will get exponential blowup if you run full recursion over all phi inputs.

Oh, so Depth is insufficient?

nikic · 2024-09-16T13:06:14Z

Yes, Depth is only sufficient for operations with low branching (generally, everything apart from phis will only need at most two recursive calls).

artagnon · 2024-09-16T13:07:05Z

What about the following then?

diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index c46152f60aab..9e173d89e129 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -3538,7 +3538,8 @@ static bool isNonEqualPHIs(const PHINode *PN1, const PHINode *PN2,
     const Value *IV2 = PN2->getIncomingValueForBlock(IncomBB);
     SimplifyQuery RecQ = Q.getWithoutCondContext();
     RecQ.CxtI = IncomBB->getTerminator();
-    if (!isKnownNonEqual(IV1, IV2, DemandedElts, Depth + 1, RecQ))
+    if (!isKnownNonEqual(IV1, IV2, DemandedElts, MaxAnalysisRecursionDepth - 2,
+                         RecQ))
       return false;
   }
   return true;

nikic · 2024-09-16T13:10:41Z

We normally only allow MaxAnalysisRecursionDepth - 1 for phis in ValueTracking.

artagnon · 2024-09-16T13:15:36Z

I see the following:

  case Instruction::PHI: {
    const PHINode *P = cast<PHINode>(Op);
    // Unreachable blocks may have zero-operand PHI nodes.
    if (P->getNumIncomingValues() == 0)
      break;

    // Otherwise take the unions of the known bit sets of the operands,
    // taking conservative care to avoid excessive recursion.
    const unsigned PhiRecursionLimit = MaxAnalysisRecursionDepth - 2;

I'll push a change, and you can check it.

artagnon · 2024-09-16T13:32:39Z

With MaxAnalysisRecursionDepth - 1, we're less powerful than before, and a test fails.

artagnon · 2024-09-23T09:13:49Z

Gentle ping. Any thoughts on whether this patch is useful?

dtcxzyw

; bin/opt -passes=early-cse reduced.ll -S
; ModuleID = 'reduced.ll'
source_filename = "reduced.ll"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

define i1 @TLSX_ValidateSupportedCurves(i1 %lnot, i32 %conv21) {
entry:
  br label %for.cond

for.cond:                                         ; preds = %for.inc, %entry
  %defSz.0 = phi i32 [ %defSz.1, %for.inc ], [ 0, %entry ]
  %nextSz.0 = phi i32 [ %nextSz.1, %for.inc ], [ 0, %entry ]
  br i1 %lnot, label %for.body, label %land.lhs.true130

for.body:                                         ; preds = %for.cond
  switch i32 %conv21, label %for.inc [
    i32 0, label %sw.epilog26
    i32 1, label %sw.epilog26
  ]

sw.epilog26:                                      ; preds = %for.body, %for.body
  br label %for.inc

for.inc:                                          ; preds = %sw.epilog26, %for.body
  %defSz.1 = phi i32 [ %defSz.0, %for.body ], [ 0, %sw.epilog26 ]
  %nextSz.1 = phi i32 [ %nextSz.0, %for.body ], [ 0, %sw.epilog26 ]
  br label %for.cond

land.lhs.true130:                                 ; preds = %for.cond
  %cmp131 = icmp eq i32 %defSz.0, %nextSz.0
  ret i1 %cmp131
}

PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      Program arguments: bin/opt -passes=early-cse test.ll -S
1.      Running pass "function(early-cse<>)" on module "test.ll"
2.      Running pass "early-cse<>" on function "TLSX_ValidateSupportedCurves"
  #0 0x00007fc029e13a92 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/bin/../lib/libLLVMSupport.so.20.0git+0x213a92)
  #1 0x00007fc029e1095f llvm::sys::RunSignalHandlers() (/home/dtcxzyw/WorkSpace/Projects/compilers/LLVM/llvm-build/bin/../lib/libLLVMSupport.so.20.0git+0x21095f)
  #2 0x00007fc029e10aa5 SignalHandler(int) Signals.cpp:0:0
  #3 0x00007fc029842520 (/lib/x86_64-linux-gnu/libc.so.6+0x42520)
  #4 0x00007fc022d29d7d isKnownNonEqual(llvm::Value const*, llvm::Value const*, llvm::APInt const&, unsigned int, llvm::SimplifyQuery const&) ValueTracking.cpp:0:0
  #5 0x00007fc022d2a218 isKnownNonEqual(llvm::Value const*, llvm::Value const*, llvm::APInt const&, unsigned int, llvm::SimplifyQuery const&) ValueTracking.cpp:0:0
  #6 0x00007fc022d2a218 isKnownNonEqual(llvm::Value const*, llvm::Value const*, llvm::APInt const&, unsigned int, llvm::SimplifyQuery const&) ValueTracking.cpp:0:0
  #7 0x00007fc022d2a218 isKnownNonEqual(llvm::Value const*, llvm::Value const*, llvm::APInt const&, unsigned int, llvm::SimplifyQuery const&) ValueTracking.cpp:0:0
  #8 0x00007fc022d2a218 isKnownNonEqual(llvm::Value const*, llvm::Value const*, llvm::APInt const&, unsigned int, llvm::SimplifyQuery const&) ValueTracking.cpp:0:0
  #9 0x00007fc022d2a218 isKnownNonEqual(llvm::Value const*, llvm::Value const*, llvm::APInt const&, unsigned int, llvm::SimplifyQuery const&) ValueTracking.cpp:0:0
 #10 0x00007fc022d2a218 isKnownNonEqual(llvm::Value const*, llvm::Value const*, llvm::APInt const&, unsigned int, llvm::SimplifyQuery const&) ValueTracking.cpp:0:0
 #11 0x00007fc022d2a218 isKnownNonEqual(llvm::Value const*, llvm::Value const*, llvm::APInt const&, unsigned int, llvm::SimplifyQuery const&) ValueTracking.cpp:0:0
...
#250 0x00007fc022d2a218 isKnownNonEqual(llvm::Value const*, llvm::Value const*, llvm::APInt const&, unsigned int, llvm::SimplifyQuery const&) ValueTracking.cpp:0:0
#251 0x00007fc022d2a218 isKnownNonEqual(llvm::Value const*, llvm::Value const*, llvm::APInt const&, unsigned int, llvm::SimplifyQuery const&) ValueTracking.cpp:0:0
#252 0x00007fc022d2a218 isKnownNonEqual(llvm::Value const*, llvm::Value const*, llvm::APInt const&, unsigned int, llvm::SimplifyQuery const&) ValueTracking.cpp:0:0
#253 0x00007fc022d2a218 isKnownNonEqual(llvm::Value const*, llvm::Value const*, llvm::APInt const&, unsigned int, llvm::SimplifyQuery const&) ValueTracking.cpp:0:0
#254 0x00007fc022d2a218 isKnownNonEqual(llvm::Value const*, llvm::Value const*, llvm::APInt const&, unsigned int, llvm::SimplifyQuery const&) ValueTracking.cpp:0:0
#255 0x00007fc022d2a218 isKnownNonEqual(llvm::Value const*, llvm::Value const*, llvm::APInt const&, unsigned int, llvm::SimplifyQuery const&) ValueTracking.cpp:0:0
Segmentation fault (core dumped)

artagnon · 2024-09-23T12:18:14Z

Thanks, taking a look.

isNonEqualPHIs was originally added in 3fd64cc ([ValueTracking] Handle two PHIs in isKnownNonEqual()), but as the new test case shows, it is not powerful enough. Generalize it, removing the APInt-specific comparison, and extend isKnownNonEqual to handle the case when one of the operands is a constant.

artagnon · 2024-09-25T11:30:14Z

The issue was a thinko, which was simple enough to fix. I think the patch should be good now.

artagnon · 2024-10-01T09:49:23Z

Gentle ping.

artagnon · 2024-10-07T09:07:49Z

Gentle ping. Seems like a straightfoward patch.

artagnon · 2024-10-21T09:14:04Z

Gentle ping.

nikic

Based on llvm-opt-benchmark, this has basically no practical impact. Given that, I'd rather not change the phi recursion handling in this function.

artagnon · 2024-10-21T12:35:55Z

Okay, I'll open a fresh PR for the new test, since I think that part is valuable.

artagnon requested a review from nikic as a code owner September 16, 2024 12:29

llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label Sep 16, 2024

nikic requested changes Sep 16, 2024

View reviewed changes

nikic mentioned this pull request Sep 23, 2024

Task submission dtcxzyw/llvm-opt-benchmark#1312

Open

dtcxzyw mentioned this pull request Sep 23, 2024

pre-commit: PR108820 dtcxzyw/llvm-opt-benchmark#1365

Closed

dtcxzyw requested changes Sep 23, 2024

View reviewed changes

artagnon added 3 commits September 24, 2024 16:36

ValueTracking: avoid compile-time blowup

fb1933c

ValueTracking: fix thinko, crash; add test

62c0f62

artagnon force-pushed the vt-nonequal-phis branch from 045d3b0 to 62c0f62 Compare September 25, 2024 11:28

dtcxzyw mentioned this pull request Sep 25, 2024

pre-commit: PR108820 dtcxzyw/llvm-opt-benchmark#1375

Closed

dtcxzyw requested a review from nikic October 7, 2024 09:14

nikic reviewed Oct 21, 2024

View reviewed changes

artagnon closed this Oct 21, 2024

artagnon deleted the vt-nonequal-phis branch October 21, 2024 12:36

artagnon mentioned this pull request Nov 5, 2024

ValueTracking/test: cover recur limit in non-equal PHIs #113205

Merged

ValueTracking: generalize isNonEqualPHIs #108820

ValueTracking: generalize isNonEqualPHIs #108820

Uh oh!

Conversation

artagnon commented Sep 16, 2024

Uh oh!

llvmbot commented Sep 16, 2024

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

artagnon commented Sep 16, 2024

Uh oh!

nikic commented Sep 16, 2024

Uh oh!

artagnon commented Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Sep 16, 2024

Uh oh!

artagnon commented Sep 16, 2024

Uh oh!

artagnon commented Sep 16, 2024

Uh oh!

artagnon commented Sep 23, 2024

Uh oh!

dtcxzyw left a comment

Choose a reason for hiding this comment

Uh oh!

artagnon commented Sep 23, 2024

Uh oh!

artagnon commented Sep 25, 2024

Uh oh!

artagnon commented Oct 1, 2024

Uh oh!

artagnon commented Oct 7, 2024

Uh oh!

artagnon commented Oct 21, 2024

Uh oh!

nikic left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artagnon commented Oct 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

artagnon commented Sep 16, 2024 •

edited

Loading

nikic left a comment •

edited

Loading