[PATCH] D65164: Define some basic terminology around loops in our documentation

Wed Jul 24 16:31:29 PDT 2019

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

Landed with most of the comments applied, but let's keep discussion going.  I'm happy to iterate further in separate patches.

================
Comment at: docs/LoopTerminology.rst:17
+the control flow graph (CFG) where there exists one block (the loop
+header block) which dominates all other blocks within the cycle.
+
----------------
kbarton wrote:
> which -> that
I ended up ignoring the which -> that suggestions.  I made the change and it seemed much less readable to me.  (And yes, I know it's technically correct.)

================
Comment at: docs/LoopTerminology.rst:23
+  dominance requirement and such are not considered loops.  LoopInfo
+  does not include such cycles.
+
----------------
Meinersbur wrote:
> jdoerfert wrote:
> > Could we add something like:
> > > commonly referred to as irreducible control flow, or irreducible loops.
> Also, such cycles are called loops in any literature that I know. It's just that LoopInfo does not create a loop object for them since they do not have a dominating header.
Can anyone provide me a citation for the definition of irreducible control flow?  I want to link to it when adding this bit.

As for the "everyone else calls them loops" points, I'm happy to mention that.  Have a suggestion for a survey paper or wikipedia which defines it?

p.s. Patch welcome to do this.  

================
Comment at: docs/LoopTerminology.rst:25-26
+
+* Loops can contain non-loop cycles and non-loop cycles may contain
+  loops.
+
----------------
Meinersbur wrote:
> The concept of a backedge should have been introduced for this. A cycle inside a loop can either be
> 
>  * A backedge
>  * A backedge of a nested loop
>  * An irreducible loop
All of our definitions are inherently circular.  I tried to adjust this a bit, what do you think of the wording I landed with?

================
Comment at: docs/LoopTerminology.rst:30
+  statically reachable from the entry of the function.  Loops which
+  become statically unreachable during optimization *must* be removed
+  from LoopInfo. 
----------------
kbarton wrote:
> Question: will the asterisks around must cause it to be typeset as bold in the documentation? 
> If so, are there other typesets available to use (e.g., italics and underline)?
*bold*, and _italics_ are the two I'm familiar with, but there are others.

================
Comment at: docs/LoopTerminology.rst:35
+  outside the loop.  A loop is allowed to be statically infinite, so
+  there need not be any exiting edges.
+
----------------
jdoerfert wrote:
> > * The loop header identifies a loop.
> > * Two loops are either disjoint or one is properly contained in the other.
> > * LoopInfo organizes loops in a tree structure with an artificial top-level loop in each function that contains all loops not contained in other loops.
The first point is is incorrect.  A single loop header can be the header of multiple nested loops.

I incorporated the second.

We should probably add a LoopInfo section; this didn't really feel like it belonged in the definition of the loops.  

================
Comment at: docs/LoopTerminology.rst:58
+predecessor inside the loop.  That is, it has a predecessor which is
+an Exiting Block.
+
----------------
kbarton wrote:
> Would a short example, instead of "That is, ..." be useful here in these definitions? If so, I can try to come up with some. 
Feel free to suggest additions/changes.  You can do that as it's own patch if you want.

================
Comment at: docs/LoopTerminology.rst:71
+the most common in the code - refers to the single out of loop
+predecessor to a loops header block.  Note that not all loops have
+such blocks.
----------------
kbarton wrote:
> I find this sentence confusing. Is this the same as: the predecessor block of the loop header that is not contained within the loop? 
> 
> I don't know that we've defined the concept of "contained within the loop" at this point, so this might not be any clearer in the long run. But, is that the general idea here?
I redrafted this completely.  What do you think of the new version?

================
Comment at: docs/LoopTerminology.rst:81
+qualification of the event as a shorthand for when some exiting block
+branches to some exit block. May be zero, or not statically computable.
+
----------------
jdoerfert wrote:
> Why "interesting event happens"?
> > The backedge*s* are traversed *without leaving the loop*
I was trying to be somewhat generic.  Technically, we can describe the BTC of any behavioural transition, not just leaving the loop.  This shows up in some of the SCEV code today.

================
Comment at: docs/LoopTerminology.rst:83-84
+
+Iteration Count - The number of times the header has executed before
+some interesting event happens.  Commonly used w/o qualification to
+refer to the iteration count at which the loop exits.  Will always be
----------------
kbarton wrote:
> Meinersbur wrote:
> > Non-rotated loops often have headers that only check the loop condition. The header executed, but if the condition evaluated to false, arguably nothing interesting happened, in particular the source language's loop body did not execute.
> Same comment as above: "The number of times the header will execute before some interesting event happens."
@Meinersbur - neither point is relevant for the llvm definition of a loop, but maybe there deserves to be a "mapping C to LLVM" section?

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65164