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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added inline comments.


================
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);
 
----------------
Meinersbur wrote:
> 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.
Most of what you said arises, IIUC, from the dominance property of SSA and makes sense. In particular:
"If x is used in the ith entry of a PHI in a block B, then the ith predecessor of B should dominate B" (I have seen this being
stated as //every predecessor of B//..., which complicates the understanding).

But I didn't understand that this implies: "The point of use of an incoming value is its incoming block."
I have never come across this anywhere and now I specifically searched for it and I couldn't find it. Is there any reference ?
If someone doesn't know about that, such code is confusing.
I'd also like to add a small mention in the LCSSA doc.


================
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)) {
----------------
Meinersbur wrote:
> 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.
True, but it's implied from the fact that `I` dominates `ExitBB` (which we know is true at this point because we can check above
and see that `ExitBB`'s are picked based on that).
This comment intends to explain "why can `I` add from every predecessor?" or "why is `I` defined in all the predecessors?". That's not
as tricky as the other one, I could just remove it.


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

https://reviews.llvm.org/D89739