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

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:20
+//
+// Note: The DEBUG_TYPE macro should be defined before using this
+// file so that any use of LLVM_DEBUG is associated with the
----------------
Not sure what this comment is for. There aren't any uses of LLVM_DEBUG in this file. And if there were, and you forgot to define DEBUG_TYPE first, you'd get a compiler error wouldn't you?


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:23
+///     (C - H). (Non-trivial means that there must be an actual cycle, i.e.
+///     a single basic blocks is only considered a cycle if it has a self-loop.)
+///
----------------
Typo "block"


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:25
+///
+/// Note: the C++ representation of the root "cycle" does _not_ contain an
+/// explicit list of blocks, to avoid unnecessary memory use.
----------------
Maybe move or copy this note to the comment on the `Blocks` field below?


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:54
+///     |   |
+///   />A] [B<\
+///   |  \ /  |
----------------
I vaguely remember that trailing backslashes in comments cause problems, but I can't remember what. Maybe some compilers warn about them?


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:139
+
+template <typename ContextT> class GenericCycle {
+public:
----------------
Doxygen comment for this class template?


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:199
+
+  bool empty() { return Entries.empty(); }
+
----------------
Does this make sense as part of the public API? How could a cycle be empty?


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:206
+  GenericCycle *getParentCycle() { return ParentCycle; }
+  uint getDepth() const { return Depth; }
+
----------------
What is uint??? Should be unsigned.


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:225
+  const_child_iterator child_begin() const {
+    return const_child_iterator{Children.begin()};
+  }
----------------
Needs whitespace inside braces (doesn't clang-format do this?).


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:298-299
+  /// A cycle structure is used here primarily to simplify the \ref GraphTraits
+  /// implementation and related iteration. Only the cycle children and entrance
+  /// member is filled in, the blocks member remains empty.
+  ///
----------------
"... and entries members are"


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:333
+  ArrayRef<BlockT *>
+  findExitBlocks(const CycleT *Cycle,
+                 SmallVectorImpl<BlockT *> &TmpStorage) const;
----------------
Needs a comment. None of the comments so far have mentioned "exit blocks".


================
Comment at: llvm/include/llvm/ADT/GenericSsaContext.h:26
+public:
+  // using BlockT = ...
+  // using InstT = ...
----------------
Should really have comments describing the intention of all the members that specializations are supposed to define.


================
Comment at: llvm/include/llvm/IR/SsaContext.h:46
+
+  Printable printableName(BasicBlock *Block) const;
+  Printable printable(Instruction *Inst) const;
----------------
Aren't Printable functions normally named like `print()` or `printFoo()`?


================
Comment at: llvm/lib/Analysis/CycleAnalysis.cpp:1
+//===- CycleInfo.cpp - IR Cycle Info ----------------------------*- C++ -*-===//
+//
----------------
Wrong file name.


================
Comment at: llvm/lib/CodeGen/MachineCycleAnalysis.cpp:1
+//===- MachineCycleInfo.cpp - Cycle Info for Machine IR ---------*- C++ -*-===//
+//
----------------
Wrong file name.


================
Comment at: llvm/lib/IR/CMakeLists.txt:1
+
 add_llvm_component_library(LLVMCore
----------------
Spurious change?


================
Comment at: llvm/lib/IR/SsaContext.cpp:11
+/// This file defines a specialization of the GenericSsaContext<X>
+/// template class for LLVM IR.
+///
----------------
It's a class template, not a template class :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112696