[PATCH] D46422: [LCSSA] Iteratively remove unused PHI nodes in formLCSSAForInstructions

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: lib/Transforms/Utils/LCSSA.cpp:233-235
+  while (RemovePHIs) {
+    RemovePHIs = false;
+    SmallSetVector<PHINode *, 16> NotRemoved;
----------------
mzolotukhin wrote:
> Could we ever iterate this loop more than once? How could we have phi-nodes to be deleted on the second iteration (~why were not they removed on the first one)?
Thanks for you feedback. I'll try to explain a little bit why I picked this solution.

In the past PHI-nodes were added to the PHIsToRemove set, while iterating over the worklist in the loop above, and then they were removed here after the loop. The problem was that a PHI node created speculatively in one such iteration, having no uses and having been inserted in the PHIsToRemove set, was used by another speculatively created PHI node created in a later iteration. So when doing the cleanup the PHI node that was targeted for removal suddenly had uses. I guess that use can either be by a PHI node that should be kept, or by a PHI node that is in the PHIsToRemove set (it can even be used by a PHI node, that is used by a PHI node, that is used by a PHI node, and some/all of those are in the PHIsToRemove list).

I do not know why we wait with the removal until after having finished with the worklist. Do you think it is possible to remove the PHI nodes directly in the first loop?

Is it guaranteed that a PHI node, without uses, being inserted in PHIsToRemove only can be used by later created PHI nodes?
Then it might be enough to iterate in reverse insertion order in the removal loop in the old solution, and changing the assert to a condition to only remove PHI nodes that still are unused (that way we would make sure that we remove the PHI node that is used by earlier candidate for removal before we try to remove it).

Another question is if it is important that we remove all unused PHI nodes?
If it is OK to do it quick and dirty, and only remove some of the speculatively created and unused PHI nodes, then we can simplify this. I guess some later pass will remove the unused PHI nodes.

My proposed patch was based on limited knowledge about the above questions. I picked a solution that removes all speculatively created PHI nodes that are unused after the first loop, including the PHI nodes that are used by PHI nodes that we remove. This solution should work regardless of the algorithm used in the first loop (such as in which order PHI nodes are created etc.). Maybe that was too ambitous.


Repository:
  rL LLVM

https://reviews.llvm.org/D46422