[PATCH] D98422: [ValueTracking] Handle two PHIs in isKnownNonEqual()

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2548
+  if (Depth >= MaxAnalysisRecursionDepth - 1)
+    return false;
+
----------------
jaykang10 wrote:
> nikic wrote:
> > Ah sorry, this isn't what I meant. What the other code does is pass `MaxAnalysisRecursionDepth - 1` to the recursive call (i.e. the recursive `isKnownNonEqual` call in this case). I just tried this, but unfortunately this would not cover your case (it needs to recurse two more levels).
> um... only one pair of phi operands can use full recursion...
> 
> ```
>   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 ConstantInt *C1 = dyn_cast<ConstantInt>(IV1);
>     const ConstantInt *C2 = dyn_cast<ConstantInt>(IV2);
>     if (C1 && C2) {
>       if (C1->getValue().eq(C2->getValue()))
>         return false;
>     } else {
>       // Only one pair of phi operands is allowed for full recursion.
>       if (UsedFullRecursion)
>         return false;
> 
>       Query RecQ = Q; 
>       RecQ.CxtI = IncomBB->getTerminator();
>       if (!isKnownNonEqual(IV1, IV2, Depth + 1, RecQ))
>         return false;
>       UsedFullRecursion = true;
>     }    
>   }
> ```
> How you you think about above one? Is it acceptable?
Yeah, that looks reasonable to me. Maybe I would write the start as:

```
const APInt *C1, *C2;
if (match(IV1, m_APInt(C1)) && match(IV2, m_APInt(C2)) && *C1 != *C2)
  continue;

if (UsedFullRecursion)
  return false;
// etc.
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98422/new/

https://reviews.llvm.org/D98422