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

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

nhaehnle added a comment.

Thank you for doing this. I'm happy with the big picture of the change obviously, though I noticed a few things inline.



================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:34
+template <typename ContextT>
+bool GenericCycle<ContextT>::contains(const GenericCycle *B) const {
+  if (!B)
----------------
Other code in this file uses the new style function definition syntax, so perhaps do so here as well to be consistent?


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:68
+
+/// \brief Helper class for computing the machine cycle information.
+template <typename ContextT> class GenericCycleInfo<ContextT>::Compute {
----------------
-machine, since this is generic code


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:14-15
+///
+/// The term "cycle" is chosen to avoid a clash with the existing notion of
+/// (natural) "loop".
+///
----------------
How about moving everything of this file comment to the new CycleTerminology document (that isn't already there) and then just refer to that document here?


================
Comment at: llvm/include/llvm/InitializePasses.h:122-123
 void initializeControlHeightReductionLegacyPassPass(PassRegistry&);
+void initializeConvergenceControlHeuristicLegacyPassPass(PassRegistry &);
+void initializeConvergenceInfoWrapperPassPass(PassRegistry &);
 void initializeCorrelatedValuePropagationPass(PassRegistry&);
----------------
These two don't belong here


================
Comment at: llvm/lib/IR/SSAContext.cpp:1
+//===- SSAContext.h ---------------------------------------------*- C++ -*-===//
+//
----------------
Wrong filename


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112696