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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Some nits, didn't follow logic fully.



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


================
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:
> 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?


================
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) {
----------------
This is only ever true if the PHI nodes are in the same basic block.



================
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);
----------------
This is potentially needlessly costly, because for switches,
each case that branches to the block, will have it's own entry in PHI node.


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

https://reviews.llvm.org/D98422