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

Mon Sep 28 06:54:08 PDT 2020

baziotis added a comment.

Thanks for taking the time to write this! I think it will help, especially beginners. I remember when I first read the loop terminology, some things were not obvious that are cleared here (e.g. why do people use backedge taken count).

================
Comment at: llvm/docs/LoopTerminology.rst:35
+ 
+* A **backedge** a an edge from a latch to the header.
+ 
----------------
[nit] "a an" -> "is an"

================
Comment at: llvm/docs/LoopTerminology.rst:51

-Key Terminology
-===============
+* The smallest loop consists of a single basic block that branches to itself. In this case that block is the header, latch (and exiting block if it has another edge to a differeny block) at the same time. A single block that has no banch to itself is not considered a loop, even though it is trivially strongly connected.
+
----------------
[nit] differeny -> different

================
Comment at: llvm/docs/LoopTerminology.rst:61
+  $ opt input.ll -loops -analyze
+  Loop at depth 1 containing: %for.body<header><latch><exiting>
+
----------------
This is a bit unexpected here. The reader might not know what the LoopInfo is or especially, its printing representation. That said, the reader can also skip it. We might want to add a reference to LoopInfo doc here.

================
Comment at: llvm/docs/LoopTerminology.rst:70
+
+* It is not possible that two loops share only a few of their nodes. Two loops are either disjoint or nested inside the other. In the example below the left and right subsets both violate the maximality condition. Only only the merge of both sets is considered a loop.
+
----------------
[nit] "or nested inside the other" -> "or one is nested inside the other"
[nit] "Only only" -> "only"

================
Comment at: llvm/docs/LoopTerminology.rst:84
+
+which might be represented in LLVM-IR as follows. Note that there os only a single header and hence just a single loop.
+
----------------
[nit] "os" -> "is"

================
Comment at: llvm/docs/LoopTerminology.rst:88
+
+The LoopSimplify pass will detect the loop and ensure separate headers for the outer and inner loop.
+
----------------
Could you please add a reference to the LoopSimplify doc below?

================
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**.  
+
----------------
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.

================
Comment at: llvm/docs/LoopTerminology.rst:101
+
+* Exiting edges are not the only way to break out of a loop. Other possibilities are unrechable terminators, [[noreturn]] functions, exceptions, and your machine's power switch.
+
----------------
[nit] "unrechable" -> "unreachable"

================
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
+
----------------
[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.

================
Comment at: llvm/docs/LoopTerminology.rst:121
+      do_something();
+      __builtin_unrechable();
+    }
----------------
[nit] "__builtin_unrechable" -> "__builtin_unreachable"

================
Comment at: llvm/docs/LoopTerminology.rst:132
+
+* There is no requirement for the control flow to eventually leave the loop, i.e. an infinite loop. A **statically infinite loop** is a loop that has no exiting edges. A **dynamically infinite loop** has exiting edges, but it is possible to be never taken. This may happen only under some circumstances, such as when n == UINT_MAX in the code blow.
+ 
----------------
"i.e. an infinite loop."

You may like this better: "i.e. a loop can be infinite"

================
Comment at: llvm/docs/LoopTerminology.rst:139
+
+It is possible for the optimizer to turn a dynamically infinite loop into a statically infinite loop, for instance when the exiting condition is false. Because the edge is never taken, the optimizer can change the conditional branch into an unconditional one.
+   
----------------
"when the exiting condition is false."

You may like this better: "when it can prove that the exiting condition is always false".

Also "Because the edge" -> "Because the exiting edge"

================
Comment at: llvm/docs/LoopTerminology.rst:141
+   
+Note that under some circumstances the compiler may assume that a loop is not infinte. A call to the intrinsic llvm.sideeffect has to be added into the loop to ensure that the optimizer does not make this assumption.
+   
----------------
[nit] "infinte" -> "infinite"

================
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:
+
----------------
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)".

================
Comment at: llvm/docs/LoopTerminology.rst:149
+   
+Since the first thing a loop header might do is to check whether there is another execution and if not, immediatly exit without doing any work (also see :ref:`loop-terminology-loop-rotate`), loop trip count is not the best measure of a loop's number of iterations. For instance, the number of header executions of of the code below for a non-positive n (before loop rotation) is 1, even though the loop body is not even executed.
+
----------------
[nit] "immediatly" -> "immediately"

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88408