[PATCH] D44564: [InstSimplify] Look at the whole PHI graph when simplifying a PHI node

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a subscriber: skatkov.
mkazantsev added a comment.

I also agree that InstCombine is expected to have `O(1)` complexity of its operations, and this code attempts to traverse the entire tree consisting of Phi nodes which is `O(N)`, and it can actually start from any node of this tree. I don't feel that this is right.



================
Comment at: lib/Analysis/InstructionSimplify.cpp:3956
+  bool HasBranchingPHI = false;
+  std::set<PHINode*> PHINodesSeen;
+  std::set<PHINode*> PendingPHINodes = { PN };
----------------
Please use `SmallPtrSet` here.


================
Comment at: lib/Analysis/InstructionSimplify.cpp:3961
+    PendingPHINodes.erase(ThisPN);
+    PHINodesSeen.insert(ThisPN);
+    // If there's some other phi node waiting to be processed then we're looking
----------------
Before that, assert that `PHINodesSeen` does not contain `ThisPN`?


================
Comment at: lib/Analysis/InstructionSimplify.cpp:3982
+          PendingPHINodes.insert(IncomingPN);
+        continue;
+      }
----------------
Why not `else if` instead of `continue`?


================
Comment at: lib/Analysis/InstructionSimplify.cpp:3989
+      }
+      if (CommonValue && Incoming != CommonValue) {
+        return BestPHINode; // Not the same, bail out.
----------------
{} not needed.


Repository:
  rL LLVM

https://reviews.llvm.org/D44564