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

[Kit Barton via Phabricator via llvm-commits]

kbarton added a comment.

I think this is really good - thanks for taking the time to start this!
Most of my comments are minor/grammatical.

If we do have access to other markups, specifically for different typesettings (bold, italics, etc.) I will suggest some more (cosmetic) changes.



================
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.
+
----------------
which -> that


================
Comment at: docs/LoopTerminology.rst:29
+* Given the use of dominance in the definition, all loops are
+  statically reachable from the entry of the function.  Loops which
+  become statically unreachable during optimization *must* be removed
----------------
which -> that


================
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. 
----------------
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)?


================
Comment at: docs/LoopTerminology.rst:46
+
+Header Block - A basic block which dominates all other blocks
+contained within the loop.  As such, it is the first one executed if
----------------
which -> that


================
Comment at: docs/LoopTerminology.rst:50
+
+Exiting Block - A basic block contained within a given loop which has
+at least one successor outside of the loop and one successor inside the
----------------
which -> that


================
Comment at: docs/LoopTerminology.rst:53
+loop.  (The latter is required for the block to be contained within the
+cycle which makes up the loop.)  That is, it has a successor which is
+an Exit Block.  
----------------
which -> that 


================
Comment at: docs/LoopTerminology.rst:56
+
+Exit Block - A basic block outside of the associated loop which has a
+predecessor inside the loop.  That is, it has a predecessor which is
----------------
which -> that


================
Comment at: docs/LoopTerminology.rst:57
+Exit Block - A basic block outside of the associated loop which has a
+predecessor inside the loop.  That is, it has a predecessor which is
+an Exiting Block.
----------------
which -> that


================
Comment at: docs/LoopTerminology.rst:58
+predecessor inside the loop.  That is, it has a predecessor which is
+an Exiting Block.
+
----------------
Would a short example, instead of "That is, ..." be useful here in these definitions? If so, I can try to come up with some. 


================
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.
----------------
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?


================
Comment at: docs/LoopTerminology.rst:74
+
+Preheader Block - A preheader is a (singular) loop predecessor which
+ends in an unconditional transfer of control to the loop header.  Note
----------------
which -> that


================
Comment at: docs/LoopTerminology.rst:79
+Backedge Taken Count - The number of times the backedge will have
+executed before some interesting event happens.  Commonly used without
+qualification of the event as a shorthand for when some exiting block
----------------
I think it's better to write this in the present tense, instead of past tense (but I don't know why I think that).

"The number of times the backedge will execute before some interesting event happens. "


================
Comment at: docs/LoopTerminology.rst:83
+
+Iteration Count - The number of times the header has executed before
+some interesting event happens.  Commonly used w/o qualification to
----------------
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."


================
Comment at: docs/LoopTerminology.rst: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
----------------
Can you expand w/o to without?


================
Comment at: docs/LoopTerminology.rst:89
+width integers (such as LLVM Values or SCEVs), you need to be cautious
+of overflow when converting one to the other.)
+
----------------
jdoerfert wrote:
> Again, we should mention "executed before the loop is left".
I think the last sentence is very important, and should not be a parenthetical. It would also be good to make Warning bold, if that's possible. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65164