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

[Ruiling, Song via Phabricator via llvm-commits]

ruiling added inline comments.


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:71
+template <typename ContextT>
+void GenericCycleInfo<ContextT>::Compute::run(BlockT *EntryBlock) {
+  assert(Info.Root->Blocks.empty());
----------------
The implementation here looks good to me. just a few comments on the naming.


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:83
+  for (BlockT *HeaderCandidate : llvm::reverse(BlockPreorder)) {
+    const DfsInfo CandidateInfo = BlockDfsInfo.lookup(HeaderCandidate);
+
----------------
I think we capitalize the term `DFS` when naming variables within LLVM? at least I found most occurrences of DFS are capitalized.


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:97
+                      << Info.Context.print(HeaderCandidate) << "\n");
+    std::unique_ptr<CycleT> Cycle = std::make_unique<CycleT>(Info);
+    Cycle->appendEntry(HeaderCandidate);
----------------
`Cycle` is too general here. What about `NewCycle` to indicate that this is the newly discovered cycle?


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:138
+        auto *BlockParent = MapIt->second;
+        auto *C = BlockParent;
+
----------------
I think it may be better to rename `C` as `OuterMostCycle` or some other reasonable name indicating that this is the outermost cycle discovered so far for this block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112696