[PATCH] D88408: [docs] Revise loop terminology reference.

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

> What do you think of the revision? Does it clarify things? Is it more understandable?

I think it is well-explained yes :) I'm trying to read it with a (more than my current) beginner knowledge and it makes sense. I added some more comments, I hope they help.

I'll also try to render this. Basically, I liked this list of terms in bold in the previous loop terminology. It just made it very easy to locate things quickly. But I think it should be as easy when they're placed
inside the text but still in bold (also as I found out, one needs to read them only a couple of times just to get acquainted, so reading them in a coherent textbook-style explanation may be better).



================
Comment at: llvm/docs/LoopTerminology.rst:54
+* An **exiting edge** is an edge from inside the loop to a node outside
+  of the loop. The source of such an edge is an **exiting block**, its
+  target is an **exit block**.
----------------
[nit] I suggest "is called" instead of just "is" (the reader might think "what is an exiting / exit block ?" while this makes clear that what they're reading is the definition).


================
Comment at: llvm/docs/LoopTerminology.rst:211
+is not infinite without proving it. For instance, it may remove a loop
+that does not anything in its body. If the loop is infinite, this
+optimization will result in an "infinite" performance speed-up. A call
----------------
[nit] "that does not anything" -> "that does not do anything" (?)


================
Comment at: llvm/docs/LoopTerminology.rst:212
+that does not anything in its body. If the loop is infinite, this
+optimization will result in an "infinite" performance speed-up. A call
+to the intrinsic :ref:`llvm.sideeffect<llvm_sideeffect>` has to be added
----------------
This is one of the best explanations for the removal of an infinite loop :)

This paragraph may raise questions though. e.g. what is the formal reasoning ? (forward progress guarantee). I think it's ok to mention here that the optimizer can do things like that without further formalities. But maybe also link to another place that explains the formalities if anyone's interested (and I don't pretend to have a good place to link for LLVM IR. Anything I know, which is not much, I've seen it on llvm-dev here and there).


================
Comment at: llvm/docs/LoopTerminology.rst:93
+
+* A cycle in the CFG does not imply there is a loop. The example below shows such a CFG, where there is no header node that dominates all other nodes in the cycle. This is called **irreducible control-flow**.  
+
----------------
Meinersbur wrote:
> baziotis wrote:
> > Do you think that adding some info on where the term irreducible comes from would be beneficial?
> > 
> > Something like:
> > Irreducible control-flow is the opposite of reducible control-flow.
> > The term reducible results from several kinds of transformations that
> > can be applied to (control-)flow (sub-)graphs that collapse sub-graphs into single nodes and, hence,
> > "reduce" the CFG successively to simpler graphs, with a CFG considered
> > to be reducible if applying a sequence of such transformations ultimately reduces
> > it to a single node. 
> > 
> > And maybe a reference if the reader is interested to learn more.
> I think I condensed you wording while still saying more. Please check whether it is still understandable.
I think it is. Because reducibility is more of a sidenote in this doc, as long as the text conveys that "reducibility is not something that you're supposed to somehow know nor is it essential to learn a lot more about it" it's good. And I think the text conveys that.


================
Comment at: llvm/docs/LoopTerminology.rst:104
+
+* A branch inside a loop that does not lead back to the loop is ot considered part of the loop, as illustrated below
+
----------------
Meinersbur wrote:
> baziotis wrote:
> > [nit] "ot" -> "not"
> > 
> > I think this example is a little bit confusing. First of all, a branch is an instruction and a loop contains basic blocks. You may mean / like this: Any BB that contains a branch that *never* (in bold to show the difference with exiting blocks, maybe even mention it in parens) leads back to the loop (or, all its successors are outside of the loop), is not considered part of the loop.
> Just "never" could also be "dynamically never" and still be part of the loop.
Right, my bad. Your updated wording is better.


================
Comment at: llvm/docs/LoopTerminology.rst:144
+
+ * The number of executions of the loop header before leaving the loop is the **loop trip count**. If the loop should not be executed at all, a **loop guard** must skip the entire loop:
+
----------------
Meinersbur wrote:
> baziotis wrote:
> > I think that the reference to the loop guard here is out of the blue it raises questions. It may be better to just leave the loop rotate part reference it.
> > 
> > Also, we might want to add "loop trip count (also called iteration count)".
> The existence of a loop guard is independent of the loop-rotated normal form, the same way a preheader may exist before LoopSimplify.
Yes, but does LoopInfo recognize it if it's not from LoopRotate ? e.g. if I write `if (cond) { do { ... } while (cond); }` in the source, will it recognize that it has a guard ? I thought that for the compiler, a guard is something only the compiler inserts while with the above wording, a guard can also exist already.

I think I explained badly my confusion when I read this: I think it's not clear what is a loop guard, who inserts it (if anyone) and when (partly this resulted from the use of "should not" and "must" above). Maybe something like that?
A guard is any block that dominates the header (it can be the header) and guards the body of the loop from not being entered if the loop condition is false before the first iteration. The compiler may insert such a guard after some transformation to ensure correctness (e.g. LoopRotate).

The wording could be better but please tell me what you think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88408