[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: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
----------------
sameerds wrote:
> 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.
 I am sorry I still don't get the point why treating nullptr as the root cycle is necessary here. And I also don't understand why "vacuous root" would help the argument here. The reason I don't want a root cycle (either a null or non-null) is it is counter-intuitive. It just makes CycleInfo different from the way LoopInfo works without any real benefits.


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:290
+                                          const CycleT *B) const {
+  if (!A)
+    return true;
----------------
sameerds wrote:
> 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`.
Could you help show me when we will passing nullptr to the first argument of contains() and we really depends on the return value should be TRUE to make caller algorithm work correctly?
Do we have many places need to check against nullptr before passing to the first argument as `if (CycleA && contains(CycleA, CycleB))`? And explicitly checking against a real-existed cycle for the first argument before calling this function sounds a clear way to me. I would even like we put an assert here that the first argument is not null:-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112696