[PATCH] D112696: CycleInfo: Introduce cycles as a generalization of loops

[Ruiling, Song via Phabricator via llvm-commits]

ruiling added inline comments.


================
Comment at: llvm/docs/CycleTerminology.rst:29
+   --- Thus, the header of the cycle is implementation-defined.)
+4. The *root cycle* consists of all basic blocks of a function. Its
+   header is the entry block of the function.
----------------
sameerds wrote:
> ruiling wrote:
> > Why do we need this `root cycle`? All the blocks of a function mostly do not even form a strongly connected region. And this is quite different from the way LoopInfo works.
> > Why do we need this `root cycle`? All the blocks of a function mostly do not even form a strongly connected region. And this is quite different from the way LoopInfo works.
> 
> I should copy a comment from GenericCycleInfo class to this spec. Essentially, the "root" is a placeholder cycle that allows CycleInfo to be treated as a single tree. Then we can use GraphTraits as follows (see `validateTree()` in GenericCycleImpl.h):
> 
> ```
> for (const CycleT *cycle : depth_first(getRoot())) { ... }
> ```
> 
My point is that the root cycle is actually not a cycle based on the definition of cycle. And If we want to check whether a block is inside any loop, we just check whether getLoopFor(block) returns nullptr, but that is not true in CycleInfo. This may be confusing for people switching from LoopInfo to CycleInfo. Am I over-concerned? I am not familiar with GraphTraits. So if we follow the same idea as LoopInfo, code will be quite complex, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112696