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

[Ruiling, Song via Phabricator via llvm-commits]

ruiling added a comment.

Thanks for this revision, the refactor make lots of sense to me. I have added some further comments. Other parts look pretty good to me.



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


================
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).
----------------
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?


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:277
+
+/// \brief Returns true iff \p A contains \p A.
+///
----------------
typo, "contains \p B".


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:290
+                                          const CycleT *B) const {
+  if (!A)
+    return true;
----------------
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.


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:83
+  for (BlockT *HeaderCandidate : llvm::reverse(BlockPreorder)) {
+    const DfsInfo CandidateInfo = BlockDfsInfo.lookup(HeaderCandidate);
+
----------------
sameerds wrote:
> ruiling wrote:
> > I think we capitalize the term `DFS` when naming variables within LLVM? at least I found most occurrences of DFS are capitalized.
> CamelCase for acronym seems to be pretty inconsistent in the LLVM codebase, and the coding standard does not say anything specific. For example, I found instances of both Cfg/CFG and Dag/DAG. I am happy to change the name it if there is a strong preference for "DFS" instead of "Dfs". But in support of my choice, CamelCase makes it easier to read "DfsInfo" compared to "DFSInfo". This is especially so when joining two acronyms, like "CfgSsa" compared to "CFGSSA".
> 
> I have this vague memory that once upon a time, the LLVM coding standard recommended that only the first letter be capitalized, but now I am not sure if I am just misremembering something else.
I think we have not explicitly require UPPERCASE for acronym in LLVM. But if you grep in LLVM source code, DFS/SSA/CFG/DAG vs Dfs/Ssa/Cfg/Dag. I think you will easily notice 90% are using the UPPERCASE. Yes, there are some occurrence of the latter version, which I don't think is a good idea. I don't like such kind of divergence situation. technically I don't have preference for one over another. But I really like most of the code could be in the same form regarding this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112696