[PATCH] D98422: [Alias] Add a ah-hoc pattern with two PHI for isKnownNonEqual

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2604
   }
+
+  // Check V1 and V2 are PHIs and they have same incoming blocks.
----------------
lebedev.ri wrote:
> I think this whole new block should be split up into it's own helper function just like `isAddOfNonZero()`.
Yep, I will move it to new helper function.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2605
+
+  // Check V1 and V2 are PHIs and they have same incoming blocks.
+  const PHINode *PN1 = dyn_cast<PHINode>(V1);
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > This is only ever true if the PHI nodes are in the same basic block.
> > 
> Add a FIXME that this is missing a generalization to handle the case where one is a PHI and another one isn't?
Yep, I will add it.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2605-2621
+  // Check V1 and V2 are PHIs and they have same incoming blocks.
+  const PHINode *PN1 = dyn_cast<PHINode>(V1);
+  const PHINode *PN2 = dyn_cast<PHINode>(V2);
+  bool HasSameIncomingBB = true;
+  if (PN1 && PN2 && Q.DT != nullptr) {
+    // Check the incoming blocks of two PHIs are same.
+    for (unsigned i = 0, e = PN1->getNumIncomingValues(); i != e; ++i) {
----------------
jaykang10 wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > This is only ever true if the PHI nodes are in the same basic block.
> > > 
> > Add a FIXME that this is missing a generalization to handle the case where one is a PHI and another one isn't?
> Yep, I will add it.
Right! I will update it.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2625-2628
+    for (unsigned i = 0, e = PN1->getNumIncomingValues(); i != e; ++i) {
+      const Value *IdxVal1 = PN1->getIncomingValue(i);
+      const BasicBlock *IncomingBB1 = PN1->getIncomingBlock(i);
+      const Value *IdxVal2 = PN2->getIncomingValueForBlock(IncomingBB1);
----------------
lebedev.ri wrote:
> This is potentially needlessly costly, because for switches,
> each case that branches to the block, will have it's own entry in PHI node.
Yep, I agree with you. I will update it.


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

https://reviews.llvm.org/D98422