[PATCH] D130891: [Docs] Improve cycle and closed path definitions

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added inline comments.


================
Comment at: llvm/docs/CycleTerminology.rst:28
+   with respect to this particular DFS. The header is always an entry node.
+4. In any depth-first search starting from the entry, the set of cycles
+   found in the CFG is the same. These are the *top-level cycles*
----------------
jsilvanus wrote:
> foad wrote:
> > jsilvanus wrote:
> > > foad wrote:
> > > > Would it make sense to swap 3 and 4, so you can define the "top-level cycles" without even mentioning DFS?
> > > You mean one could define "top-level cycles" without mentioning a particular DFS choice?
> > > 
> > > The current order has the advantage that entries and headers are adjacent, resembling their close relationship.
> > > But I do not have a strong opinion here.
> > > You mean one could define "top-level cycles" without mentioning a particular DFS choice?
> > 
> > Yes. The sentence "In any depth-first search starting from the entry, the set of cycles found in the CFG is the same" is really strange. Why would you even mention DFS here??? It should be clear from (1) that cycles have nothing to do with any DFS ordering. It's like saying "the set of cycles found in the CFG is the same regardless of whether Denmark and Canada share a land border" :)
> > 
> > I guess the only reason that (4) mentions a DFS is that (3) has already mentioned DFS. That's why I suggested reordering them. But perhaps we could keep the order the same and still remove DFS from (4)?
> Ah, now I understand, and I think you are right.
> 
> The current wording probably originates from something like "The set of top-level cycles (i.e. cycles in the original graph) can be found by DFS. In particular, the set of cycles found this way does not depend on the DFS choice."
> 
> What about moving 4 to before 2 (to keep 2 and 3 together), and rephrasing to:
> 
> The set of cycles in the function CFG itself (and not a nontrivial subgraph of it) are called *top-level cycles*.
> The top-level cycles can be found using a DFS from the entry node.
We could make it simpler by not focusing so much on "any DFS". What if that part is moved to section titled "Effect of DFS on Cycle Hierarchy"? Under that, we can now say that the top-level cycles are always the same, but the choice of header results in different hierarchies inside the top-level cycles.

This effect on DFS is not necessary to define how cycles are found. So this enumerated definition can just say DFS and "first node encountered is the header". The choice of header is important for its effect on the users of the cycles found. Having it in a separate section with its own anchor will make it very convenient to add refs in other places like the uniformity info document.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130891