[PATCH] D89739: [LCSSA] Doc for special treatment of PHIs

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

The additional test might be useful though.



================
Comment at: llvm/lib/Transforms/Utils/LCSSA.cpp:128-129
+      // but not the second.
       if (auto *PN = dyn_cast<PHINode>(User))
         UserBB = PN->getIncomingBlock(U);
 
----------------
This is not specific to LCSSA, but a general rule: The point of use of an incoming value is its incoming block. That is, the definition (the `I`) must dominate `UserBB` (and not be basic block where the PHI is located). As a consequence, a use of the PHI in the exit block of the loop is inside the loop.

This becomes more obvious by MLIR's basic block arguments: the basic block that branches to the destination specifies the arguments for the "PHI" basic block parameters.

This additional comment makes it more complicated and special than it is.


================
Comment at: llvm/lib/Transforms/Utils/LCSSA.cpp:177-178
       PN->setDebugLoc(I->getDebugLoc());
-      // Add inputs from inside the loop for this PHI.
+      // Add inputs from inside the loop for this PHI. This is valid
+      // because `I` dominates `ExitBB`.
       for (BasicBlock *Pred : PredCache.get(ExitBB)) {
----------------
baziotis wrote:
> Ah, I just saw that. Do you want me to put it in a separate diff ?
What does the comment addition intend to explain? Technically, not the ExitBB must be dominated by `I`, but its predecessors.


================
Comment at: llvm/lib/Transforms/Utils/LCSSA.cpp:214-220
+
+      // A PHI may already act as an LCSSA PHI as explained in the gathering
+      // of uses to rewrite (note that here there are possibly other PHI's
+      // included when we added LCSSA PHI's in the exit blocks, after the
+      // gathering of uses)
       if (auto *PN = dyn_cast<PHINode>(User))
         UserBB = PN->getIncomingBlock(*UseToRewrite);
----------------
Same comment as before applies.


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

https://reviews.llvm.org/D89739