[PATCH] D64355: [docs][Remarks] Add documentation for remarks in LLVM

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

Added a few style suggestions (prefixed with "Suggestion") which aren't critical.

Also added a couple comments on things that are missing documentation or are ambiguous.



================
Comment at: llvm/docs/Remarks.rst:57-58
+
+There are two modes that are supported for enabling optimization remarks in
+LLVM.
+
----------------
Suggestion:

It might seem redundant, but I would add a sentence just saying what those two modes are. That would tell the reader about what they're going to find in this section.


================
Comment at: llvm/docs/Remarks.rst:63-65
+While having diagnostics propagated from the middle-end or back-end of the
+compiler is fairly rare, optimization remarks can be enabled to be emitted as
+diagnostics. These diagnostics will be propagated to front-ends if desired, or
----------------
Suggestion:

I think this sentence is somewhat confusing. The first part kind of jumbled my brains a bit and had me thinking that //optimization remarks// aren't often produced by the middle/back end.

Maybe just drop the first half  of the sentence? E.g.

"Optimization remarks can be emitted as diagnostics. These diagnostics will..."


================
Comment at: llvm/docs/Remarks.rst:87
+
+Serialized remarks
+------------------
----------------
Should we call this an "optimization record" instead of serialized remarks? Later in the document, (in the opt-viewer section), you mention optimization records.

It would be good to adopt one term, or at least define both.


================
Comment at: llvm/docs/Remarks.rst:94-95
+
+For that, LLVM can serialize the remarks produced for each compilation unit to
+a file that can be consumed later.
+
----------------
Suggestion:

I think that it would be good to mention the later YAML and object file sections here, and link to them (even though they are directly below.) That would improve the flow of the section by giving the reader a heads-up.


================
Comment at: llvm/docs/Remarks.rst:97-98
+
+:doc:`llc <CommandGuide/llc>` and :doc:`opt <CommandGuide/opt>` support the
+following options:
+
----------------
Suggestion:

I think there are two themes here:

1) Output style, format, etc.
2) Controlling the contents of the output via filtering, thresholding etc.

Would there be any nice way to split this up into those two themes? I think it would make it really easy to skim the options, even though there aren't many of them right now.


================
Comment at: llvm/docs/Remarks.rst:102
+
+      Enables the serialization of remarks to a file specified in <filename>.
+
----------------
This is always a YAML file, right? Might be good to mention that here.


================
Comment at: llvm/docs/Remarks.rst:164-167
+* ``<TYPE>``
+* ``<pass>``
+* ``<name>``
+* ``<function>``
----------------
Can you add descriptions for these?


================
Comment at: llvm/docs/Remarks.rst:169
+
+If a ``DebugLoc`` entry is specified, all the following fields are required:
+
----------------
Suggestion: Drop the "all" to be consistent with the other lists below.


================
Comment at: llvm/docs/Remarks.rst:180
+
+If a ``DebugLoc`` entry is specified within an argument, the following fields
+are required:
----------------
Suggestion: Replace "argument" with "arg entry", like above. "Argument" looks like it could be something else.


================
Comment at: llvm/docs/Remarks.rst:190-191
+
+The ``opt-viewer`` tool generates HTML output to visualize optimization records
+from the YAML files generated with the appropriate flags described above.
+
----------------
2/3 of the tools listed in this section do not produce HTML output.

Maybe say that the opt-viewer //directory// contains a collection of tools that visualize and summarize optimization records?


================
Comment at: llvm/docs/index.rst:187
+:doc:`Remarks`
+   Notes on the implementation of the remark diagnostics in LLVM.
 
----------------
I think it would be better to call this a reference. It's not really notes, and it's not //just// diagnostics.



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

https://reviews.llvm.org/D64355