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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds marked 7 inline comments as done.
sameerds added inline comments.


================
Comment at: llvm/docs/CycleTerminology.rst:85
+
+2. If a node D dominates one or more nodes in a closed path P, then
+   there exists a cycle C that contains P but not D.
----------------
foad wrote:
> I think this needs to say "D **properly** dominates", with the usual convention that a block dominates itself but does not properly dominate itself. (We don't seem to have a domination terminology doc.)
> I think this needs to say "D **properly** dominates", with the usual convention that a block dominates itself but does not properly dominate itself. (We don't seem to have a domination terminology doc.)

Fixed this by requiring that D is not contained in P. The statement is not true if D is in P!


================
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
----------------
arsenm wrote:
> foad wrote:
> > sameerds wrote:
> > > foad wrote:
> > > > 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?
> > > I am not sure what the objection is. This comment simply explains what to do if there is an LLVM_DEBUG occurrence. This file specifically does not define DEBUG_TYPE so that the messages can be enabled by the file that actually specializes these templates. It so happens that this particular file does not contain any LLVM_DEBUG messages, but I am  using this as a standard header for more such files in the pipeline.
> > No strong objection, it just seems like the kind of comment you could put in any header file in the whole project, which suggests to me that it's not adding much value.
> I thought you had to define LLVM_DEBUG after any includes to avoid some issue
Fixed all of this by defining DEBUG_TYPE in the template implementation itself. This is closer to how the macro is defined in other header files. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112696