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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Some resources that might help:

https://reviews.llvm.org/D18443
https://lists.llvm.org/pipermail/llvm-dev/2016-February/095964.html
https://github.com/llvm/llvm-project/commit/ff379b69b2f82c9ca9b955b0458bb1aba85b9b85
https://llvm.org/docs/LangRef.html#id311



================
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);
 
----------------
baziotis wrote:
> 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.
> But I didn't understand that this implies: "The point of use of an incoming value is its incoming block."

My statement is somewhat incorrect, the "use" of a value by a PHI is its incoming edge:

https://llvm.org/docs/LangRef.html#phi-instruction
> For the purposes of the SSA form, the use of each incoming value is deemed to occur on the edge from the corresponding predecessor block to the current block.

For practical purposes (e.g. to check dominance requirements), it is easier to view the incoming block as where the use occurs, as edges do not contain code itself.  

This is a common technique, not just for LCSSA, such as in:
https://github.com/llvm/llvm-project/blob/c565f09f4b0d908f51aaf4a841285f39ef93bc8c/llvm/lib/Analysis/LoopInfo.cpp#L434
https://github.com/llvm/llvm-project/blob/c565f09f4b0d908f51aaf4a841285f39ef93bc8c/llvm/lib/CodeGen/CodeGenPrepare.cpp#L1173
https://github.com/llvm/llvm-project/blob/4aa97e3dacf3bdf5636fbf89dd8c64f1e4648065/polly/lib/Support/ScopHelper.cpp#L665


================
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:
> 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.
To be useful, the comment should elaborate more on that.

That `ExitBB` is dominated by `I` is checked at the beginning of the loop (line #166). This implies that every incoming block/edge is dominated by `I` as well, i.e. we can add uses of `I` to those incoming edges/append to the the incoming blocks without violating the SSA dominance requirement.

I think you want and answer to the "This implies ..." part?


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

https://reviews.llvm.org/D89739