[PATCH] D75233: [LoopTerminology] LCSSA Form

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis marked 3 inline comments as done.
baziotis added a comment.

In D75233#1943883 <https://reviews.llvm.org/D75233#1943883>, @Meinersbur wrote:

> In D75233#1932574 <https://reviews.llvm.org/D75233#1932574>, @baziotis wrote:
>
> > 2 questions:
> >
> > 1. Is it "loop closed PHI node" or "loop closing PHI node"?
>
>
> Not being an expert of the English language, I;d say "loop-closing PHI node"
>
> > 2. Does LCSSA pass have to do this deep analysis? Because to me, it seems it can just use `RAUW`?
>
> The most complicated thing is finding the dominator nodes to add the PHI nodes in (especially with multiple loop exits).
>
> If possible, could you avoid "deep analysis", but instead refer the kind of analysis needed?


Yes. Could you explain the part about dominator nodes ? I understand how dominators help in the identification
of loops and how dominance frontiers help in the placement of normal PHI nodes (i.e. when 2 or more paths converge). But I don't see how dominators
help with the placement of LCSSA phi nodes.



================
Comment at: llvm/docs/LoopTerminology.rst:261-264
+to take effect.  However, we should note one thing:
+LLVM keeps all the uses of a value in an internal data structure.
+That means we don't have to do deep analysis to find and replace
+all uses of a value, we can just iterate this data structure (there
----------------
Meinersbur wrote:
> You you replace "deep analysis" by "data flow analysis"? Also mention this the use-def list is a property of SSA in general. Without mentioning these terms, I had to read this paragraph 3 times before I understood what ou want to say.
Yes.

> Without mentioning these terms, I had to read this paragraph 3 times before I understood what ou want to say.
Ok, I'll remove the LLVM-specific part (internal data structure for uses etc.) and I'll focus on the things that we know from SSA anyway (def-use chain).


================
Comment at: llvm/docs/LoopTerminology.rst:266
+is even a utility function, replaceAllUsesWith(), that performs
+this transformation).
+
----------------
Meinersbur wrote:
> If you think it's worth mentioning, you could add that it's also easy to find all values that are used outside the loop: iterate over the PHINodes in the exit nodes (instead looking of all use-lists of all instructions in the loop).
I added it above. It's the first benefit. But I didn't add the "instead" part, I'll add it.


================
Comment at: llvm/docs/LoopTerminology.rst:285-287
+base 100 and step 1.  Although, in practice, and in the LLVM context,
+SCEV should have no problem with it (it's just two AddRecExpr
+in a SCEVExpr).
----------------
Meinersbur wrote:
> Could you add a sentence introducing SCEV? In this sentence it appears without context.
Yes, I should somehow add it before these 2 paragraphs.


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

https://reviews.llvm.org/D75233