[PATCH] D142551: [InstCombine] Teach isDeadPHICycle to look through multiple uses

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:1007
                            SmallPtrSetImpl<PHINode *> &PotentiallyDeadPHIs) {
-  if (PN->use_empty()) return true;
-  if (!PN->hasOneUse()) return false;
+  if (PHINode *PN = dyn_cast<PHINode>(I)) {
+    // A PHI node with no uses is dead.
----------------
This code should also run for non-phis, with PotentiallyDeadPHIs -> PotentiallyDeadInsts. We want to count non-phi instructions towards the limit as well. I expect this is why you're seeing such a large compile time impact, because as written we only limit the number of visited phis, not the number of visited instructions.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:1023
+  // also in a cycle.
+  if (I->use_empty() || I->mayHaveSideEffects())
+    return false;
----------------
Returning `false` for `I->use_empty()` doesn't really make sense. I suspect that what you're trying to work around here is the something like `br` doesn't have side effects, but also can't just be removed. The correct predicate to check here is `wouldInstructionBeTriviallyDead`.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:1027
+  return all_of(I->users(), [&PotentiallyDeadPHIs](User *U) {
+    Instruction *UI = dyn_cast<Instruction>(U);
+    if (!UI)
----------------
Can `cast<>` here, all users of instructions are instructions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142551