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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added inline comments.


================
Comment at: llvm/docs/LoopTerminology.rst:297
+in LCSSA. To achieve the latter, for each value that is live across the
+loop boundary, single entry PHI nodes to each of the exit blocks
+[#lcssa-construction]_ in order to "close" these values inside the loop.
----------------
Meinersbur wrote:
> verb before "single" missing?
Yeah thanks. Actually the best place is probably after "PHI nodes" i.e. "PHI nodes are inserted"


================
Comment at: llvm/docs/LoopTerminology.rst:463-464
+  can't be used from the PHI and onwards (otherwise the PHI would not
+  be needed in the first place). Effectively, the PHI kills the incoming
+  value(s) and replaces them with a new definition.
+
----------------
Meinersbur wrote:
> Mmmh, more precisely, the dominance frontier will not allow uses of the value past it, unless it is passed on by a PHI node. Mem2Reg tries to minimize the number of PHIs, only adding them at dominance frontiers, but is not strictly required to (I am not sure whether the current implementation does). With LCSSA, we intentionally add PHIs even though we are not crossing a dominance frontier. 
> 
> I.e. it's not the PHI that kills a value and it is structurally valid (although optimizable) to use PHI as well as the incoming value as long as the dominance requirement is fulfilled. That is, both lifetimes may overlap (however if crossing a loop, that would obviously not be LCSSA normal form anymore).
Yes, I totally agree! Yes formally one is allowed to re-use the value if it dominates the block, but if e.g. a human was hand-writing the
code, they wouldn't probably use both values: Either they'd use the value because it dominates or they'd need to put a PHI (which 
would kill in their head the previous value). Even if they put an artificial PHI for some reason (e.g. LCSSA), they would still probably
not use the previous value, but instead the PHI.

Bottom-line, intuitively, the human would consider the value "live" and / or "used" up to the predecessor block, which is why IMHO this analogy is helpful for intuition.

That said, if you like the analogy, I still have to rewrite that somehow, on the one hand avoiding much formal details (because the goal of this is intuition) OTOH without writing incorrect things since this is now incorrect. How does that sound?


================
Comment at: llvm/docs/LoopTerminology.rst:223-225
+corresponding predecessor block to the current block". To that end,
+formally the use truly happens at the boundary of the loop and for
+the purposes of LCSSA, we consider it happens inside it.
----------------
Meinersbur wrote:
> baziotis wrote:
> > Meinersbur wrote:
> > > The edge is clearly outside the loop, since when the control-flow uses it, it will not be looping anymore.
> > > 
> > > Treating the exiting block as user moves the use into the loop. For the purpose of LCSSA, this is exactly be what we want, since ensures all values that are defined in a loop are also only used in the loop. Could you move that part to the following paragraph to separate its from the strict formalism?
> > Hmm, it seems to me that this will throw people off. It is easy to think "if the edge is considered outside how do you consider the use inside?" Let's assume that we answer that with "it works for LCSSA's purpose". But then "this is used all over LLVM, not just LCSSA". The whole reasoning seems flawed and like a hack. I'll retry anyway.
> > 
> > (Note that this part of the paragraph tries to answer what the start of the paragraph questions; if I move it, I'll have to rewrite all of it, which I'll do).
> Note that treating the use to occur in the incoming block just "overapproximates" the number of uses. The incoming block could branch to a different block. In that case the incoming value hasn't been uses, but there is no side-effect(*).
> 
> (*) Theoretically it could extend the lifetime of that value, but we do not compute lifetimes in LCSSA.
Oh that's another very helpful way to think about this, thanks!


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

https://reviews.llvm.org/D89739