[PATCH] D83094: Analysis: Add a GenericCycleInfo analysis

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/Analysis/GenericCycleInfo.h:149
+  /// Child cycles, if any.
+  std::vector<std::unique_ptr<GenericCycleBase>> m_children;
+
----------------
SmallVector?


================
Comment at: llvm/include/llvm/Analysis/GenericCycleInfo.h:153
+  /// and including blocks that are part of a child cycle.
+  std::vector<CfgBlockRef> m_blocks;
+
----------------
SmallVector?


================
Comment at: llvm/include/llvm/Analysis/GenericCycleInfo.h:160
+  ///       always have the same depth.
+  uint m_depth = 0;
+
----------------
Not sure where this uint typedef is coming from


================
Comment at: llvm/include/llvm/Analysis/GenericCycleInfo.h:479
+
+  // Not implemented:
+  // static nodes_iterator nodes_begin(GraphType *G)
----------------
Is this in a future patch?


================
Comment at: llvm/lib/Analysis/GenericCycleInfo.cpp:80
+    // Found a cycle with the candidate as its header.
+    auto cycle = std::make_unique<GenericCycleBase>();
+    cycle->m_entries.push_back(headerCandidate);
----------------
I think auto hurts here, and this should explicitly be std::unique_ptr


================
Comment at: llvm/lib/Analysis/GenericCycleInfo.cpp:99
+      if (isEntry) {
+        assert(llvm::find(cycle->m_entries, block) == cycle->m_entries.end());
+        cycle->m_entries.push_back(block);
----------------
I think there is an is_contained for this pattern


================
Comment at: llvm/lib/Analysis/GenericCycleInfo.cpp:136
+        m_info.m_blockMap.try_emplace(block, cycle.get());
+        assert(llvm::find(cycle->m_blocks, block) == cycle->m_blocks.end());
+        cycle->m_blocks.push_back(block);
----------------
!is_contained


================
Comment at: llvm/lib/Analysis/GenericCycleInfo.cpp:178
+      bool added = m_blockDfsInfo.try_emplace(block, ++counter).second;
+      assert(added);
+      m_blockPreorder.push_back(block);
----------------
Unused variable warning in release build


================
Comment at: llvm/lib/Analysis/GenericCycleInfo.cpp:192-200
+/// \brief Return whether \p block is an entry block of the cycle.
+bool GenericCycleBase::isEntry(CfgBlockRef block) const {
+  return llvm::find(m_entries, block) != m_entries.end();
+}
+
+/// \brief Return whether \p block is contained in the cycle.
+bool GenericCycleBase::containsBlock(CfgBlockRef block) const {
----------------
Might as well put these in the header?


================
Comment at: llvm/lib/Analysis/GenericCycleInfo.cpp:344
+
+/// \brief Find the smallest cycle containing both \p a and \p b.
+GenericCycleBase *
----------------
I expect documentation comments on the declarations in the header, not in the implementation


================
Comment at: llvm/lib/Analysis/GenericCycleInfo.cpp:427
+/// or that the right set of cycles in the CFG were found.
+void GenericCycleInfoBase::validateTree() const {
+  DenseSet<CfgBlockRef> blocks;
----------------
I think it would be more helpful to have this return a bool for fail/pass, and not directly assert. The assert conditions could print more about why it's not valid (although there so many asserts, this might be annoying)


================
Comment at: llvm/lib/Analysis/GenericCycleInfo.cpp:439-440
+      assert(cycle != getRoot());
+      assert(llvm::find(cycle->m_parent->children(), cycle) !=
+             cycle->m_parent->children_end());
+
----------------
is_contained


================
Comment at: llvm/lib/Analysis/GenericCycleInfo.cpp:444
+        auto mapIt = m_blockMap.find(block);
+        assert(mapIt != m_blockMap.end());
+        assert(contains(cycle, mapIt->second));
----------------
Unused variable in release


================
Comment at: llvm/lib/Analysis/GenericCycleInfo.cpp:453
+        assert(entries.insert(entry).second);
+        assert(llvm::find(cycle->m_blocks, entry) != cycle->m_blocks.end());
+      }
----------------
is_contained


================
Comment at: llvm/lib/Analysis/GenericCycleInfo.cpp:472
+         cycle = cycle->m_parent) {
+      assert(llvm::find(cycle->m_blocks, block) != cycle->m_blocks.end());
+    }
----------------
is_contained


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83094