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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

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


================
Comment at: llvm/docs/UserGuides.rst:30
    CoverageMappingFormat
+   CycleTerminology
    DebuggingJITedCode
----------------
foad wrote:
> Judging by the raw diff, you have introduced some Windows-style line endings into this file.
I thought so too, but this original source file is DOS formatted, and Emacs faithfully added the Windows-style new lines.


================
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
----------------
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.


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:54
+///     |   |
+///   />A] [B<\
+///   |  \ /  |
----------------
foad wrote:
> I vaguely remember that trailing backslashes in comments cause problems, but I can't remember what. Maybe some compilers warn about them?
An internet search revealed that things get interesting when a single-line comment has a trailing backslash and useful code on the next line. The backslash eats up the new-line , and on some platforms, depending on the definition of new-line, the useful code is eaten up by the comment.

Clearly, no such issue here inside a block of single-line comments.


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:225
+  const_child_iterator child_begin() const {
+    return const_child_iterator{Children.begin()};
+  }
----------------
foad wrote:
> Needs whitespace inside braces (doesn't clang-format do this?).
Those braces enclose an initializer list and not a block of statements. Hence no space inside them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112696