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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds marked 6 inline comments as done and an inline comment as not done.
sameerds 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.
----------------
ruiling wrote:
> 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?
I see now. Thanks for pointing out the parallel with LoopInfo, it might be useful in the future. I've now eliminated the root cycle. Luckily, it did not cause much side-effects in other places.


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:83
+  for (BlockT *HeaderCandidate : llvm::reverse(BlockPreorder)) {
+    const DfsInfo CandidateInfo = BlockDfsInfo.lookup(HeaderCandidate);
+
----------------
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.


================
Comment at: llvm/lib/CodeGen/MachineSsaContext.cpp:43
+
+    if (Value) {
+      Out << ": ";
----------------
critson wrote:
> This needs to be able to deal with physical registers.
> 
> 
Does the new check look better? From the implementation, it seems that getUniqueVRegDef() will gracefully handle physical registers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112696