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

Stefanos Baziotis via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Oct 21 01:06:40 PDT 2020


baziotis added a comment.

> Some resources that might help:

Thanks, I totally missed that it is in the langref. Off-topic: In the cases targeted by that diff / fix to the verifier, I think it is also interesting to consider that PHI's are evaluated in parallel upon entry to a BB.



================
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:
> 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
Thanks for the feedback, so this is not something imposed by SSA, just something that happens to be useful. Good.
I think it's better to shorten the comment and I'll move some of it 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:
> > 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?
I'm not sure whether it should be that verbose to be useful (e.g. I had never read LCSSA and that part was quite clear when I realized that `I` dominates `ExitBB`). No problem though, I'm adding it.


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

https://reviews.llvm.org/D89739



More information about the llvm-commits mailing list