[PATCH] D83094: Analysis: Add a GenericCycleInfo analysis

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle marked 17 inline comments as done.
nhaehnle added a comment.

Thank you for the feedback. I've addressed things locally, will update the patch once I've gone through the whole series (probably on Monday)



================
Comment at: llvm/include/llvm/Analysis/CycleInfo.h:19
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/Dominators.h"
+#include "llvm/IR/PassManager.h"
----------------
RKSimon wrote:
> Do you need to include BasicBlock + Dominators? Can you use forward declarations instead?
Cleaned up locally.


================
Comment at: llvm/include/llvm/Analysis/GenericCycleInfo.h:129
+#include "llvm/Support/GenericDomTree.h"
+#include "llvm/Support/Printable.h"
+
----------------
RKSimon wrote:
> Do you need to include all these? Can you use forward declarations got any instead?
GenericDomTree was a left-over, removed locally.


================
Comment at: llvm/include/llvm/Analysis/GenericCycleInfo.h:149
+  /// Child cycles, if any.
+  std::vector<std::unique_ptr<GenericCycleBase>> m_children;
+
----------------
arsenm wrote:
> SmallVector?
This is following `LoopBase`, which uses a std::vector for child loops. I think it makes sense, given that most loops probably don't have any child loops.

`m_entries` is different: `LoopBase` doesn't have an equivalent, and we'd expect almost all loops to be natural loops, i.e. with exactly one entry.


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


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


================
Comment at: llvm/include/llvm/Analysis/GenericCycleInfo.h:479
+
+  // Not implemented:
+  // static nodes_iterator nodes_begin(GraphType *G)
----------------
arsenm wrote:
> Is this in a future patch?
No. I found it hard to implement these kind of templates correctly without code that actually uses them, and since LoopInfo doesn't implement those parts of GraphTraits either, well...


================
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);
----------------
arsenm wrote:
> I think auto hurts here, and this should explicitly be std::unique_ptr
You mean in terms of readability? I thought it'd be clear because the type is on the RHS, but in any case I've changed it locally.


================
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);
----------------
arsenm wrote:
> I think there is an is_contained for this pattern
TIL. Locally replaced this throughout my changes.


================
Comment at: llvm/lib/Analysis/GenericCycleInfo.cpp:344
+
+/// \brief Find the smallest cycle containing both \p a and \p b.
+GenericCycleBase *
----------------
arsenm wrote:
> I expect documentation comments on the declarations in the header, not in the implementation
Hmm, I've found different precedents for this in LLVM, e.g. IRBuilder.cpp has a bunch of documentation comments on methods.

Personally, I find it more convenient in the source file: IDEs generally pick the comments up either way, and this keeps the header files leaner which allows the reader to get a quicker overview of what the class offers.


================
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;
----------------
arsenm wrote:
> 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)
It's not clear to me why the fail/pass return would be more helpful?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83094