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

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2548
+  if (Depth >= MaxAnalysisRecursionDepth - 1)
+    return false;
+
----------------
nikic wrote:
> 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.
> ```
Yep, It looks better. I will update it.


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

https://reviews.llvm.org/D98422