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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds marked 3 inline comments as done.
sameerds added inline comments.


================
Comment at: llvm/docs/CycleTerminology.rst:33-34
+   --- wherever applicable, the implementation interprets ``nullptr``
+   as if it were a vacuous *root cycle* defined to be the parent of
+   all top-level cycles.)
+5. The cycles nested inside a cycle C with header H are the outermost
----------------
ruiling wrote:
> It would be better we are not treating nullptr as the root-cycle unless we really have to. My understanding is that nullptr is just saying that this is the outermost cycle, it does not have any parent.
Not everything can be measured with the metric of "unless we have to". Treating `nullptr` as the "vacuous root" (please note the term "vacuous") is worth the convenience it provides.

This is not a novelty, it happens often in formal definitions, including the paper by Havlak that this implementation is based on. Such vacuous objects are used to make life easy for inductive reasoning about formalisms. To look at it differently, think of it as `0!` defined as `1` just to make it easy to write an inductive definition of the factorial function.


================
Comment at: llvm/docs/CycleTerminology.rst:64
+A basic block or cycle X is a *sibling* of another basic block or
+cycle Y if they both have the same parent (can be ``nullptr`` in the
+case of top-level cycles).
----------------
ruiling wrote:
> I am not good at English wording. But is it meaningful to say two basic blocks are siblings to each other if both of them are not in any cycle?
It's not about the language used. This section defines terms, and I am **//defining//** what a sibling is when neither of them are in a "real" cycle because they are both in a "vacuous" root cycle.


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:290
+                                          const CycleT *B) const {
+  if (!A)
+    return true;
----------------
ruiling wrote:
> While it is reasonable to return nullptr when querying parent cycle of top level cycle. It does not sound reasonable that we say nullptr contains an ordinary cycle. Do you have any use case that can demonstrate we need to define the behavior of contains() to `return true if input A is nullptr` to get expected result? I don't know what the case you have shown `contains(getCycle(F), getCycle(G)` used for. IMO, null input value is just non-applicable here, we should just return false if either A or B is nullptr.
While the code does not contain an explicit call to `contains(getCycle(F), getCycle(G)`, it does contain calls to `contains()` where one of the arguments became null just because the caller went up the hierarchy. It is much more consistent to handle this situation inside `contains()` than adding `if (!cycle) { // then that means we are the toplevel }` in multiple places.

Where it matters, I have used `Optional<Cycle*>` to differentiate between "no cycle" and "root cycle". I believe that my interpretation of `nullptr` is convenient enough that I am willing to go use the `Optional<>` type where it needs to be disambiguated. In fact, that is the very purpose for the existence of types like `llvm::Optional<>`, `std::opt`, and Haskell `Maybe`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112696