Page MenuHomePhabricator

[docs][Remarks] Add documentation for remarks in LLVM
ClosedPublic

Authored by thegameg on Jul 8 2019, 11:03 AM.

Details

Summary

This adds documentation that describes remarks in LLVM.

It aims at explaining what remarks are, how to enable them, and what users can do with the different modes.

It lists all the available flags in LLVM (excluding clang), and describes the expected YAML structure as well as the tools that support the YAML format today.

Diff Detail

Repository
rL LLVM

Event Timeline

thegameg created this revision.Jul 8 2019, 11:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2019, 11:03 AM
Herald added a subscriber: arphaman. · View Herald Transcript
paquette added inline comments.Jul 8 2019, 11:16 AM
llvm/docs/Remarks.rst
36 ↗(On Diff #208458)

Probably meant to write foo here? (Even though it's a dummy name, and foz is pretty good anyway)

72 ↗(On Diff #208458)

Should probably mention that this is a POSIX regex.

119 ↗(On Diff #208458)

Link to PGO documentation so that people can look at terminology for profiling etc.?

240 ↗(On Diff #208458)

opt-diff is really *only* intended for a changing compiler + fixed source, right? "E.g." doesn't really pack enough punch here; we should probably just say that the tool isn't intended for a changing source + changing compiler.

Thank your for working on the documentation!

Are you completing the FIXME's before committing?

llvm/docs/Remarks.rst
97 ↗(On Diff #208458)

Clang does as well using -mllvm

282 ↗(On Diff #208458)

Is this meant to use = (assignment instead of comparison)?

thegameg updated this revision to Diff 208482.Jul 8 2019, 12:48 PM
thegameg marked 6 inline comments as done.
thegameg added a reviewer: Meinersbur.

Address Jessica and Michael's comments. Thanks!

llvm/docs/Remarks.rst
97 ↗(On Diff #208458)

I initially didn't want to mention clang at all in the docs here, but if you think it's useful I can add the missing clang flags as well.

119 ↗(On Diff #208458)

I tried looking for a link, but I can't find anything else than https://llvm.org/docs/HowToBuildWithPGO.html...

282 ↗(On Diff #208458)

It is indeed!

Are you completing the FIXME's before committing?

I was going to leave that for later. If it's an issue I can hide the empty sections from the final output.

thegameg marked 2 inline comments as done.Jul 8 2019, 12:53 PM
thegameg added inline comments.
llvm/docs/Remarks.rst
240 ↗(On Diff #208458)

I'll mention both use-cases:

  • changing compiler + fixed source
  • changing source + fixed compiler
thegameg updated this revision to Diff 208488.Jul 8 2019, 12:57 PM
thegameg marked an inline comment as done.

Update the opt-diff use cases.

Hiding the empty sections sounds like a good call to me.

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.

llvm/docs/Remarks.rst
57–58 ↗(On Diff #208488)

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.

63–65 ↗(On Diff #208488)

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

87 ↗(On Diff #208488)

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.

94–95 ↗(On Diff #208488)

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.

97–98 ↗(On Diff #208488)

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.

102 ↗(On Diff #208488)

This is always a YAML file, right? Might be good to mention that here.

164–167 ↗(On Diff #208488)

Can you add descriptions for these?

169 ↗(On Diff #208488)

Suggestion: Drop the "all" to be consistent with the other lists below.

180 ↗(On Diff #208488)

Suggestion: Replace "argument" with "arg entry", like above. "Argument" looks like it could be something else.

190–191 ↗(On Diff #208488)

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?

llvm/docs/index.rst
187 ↗(On Diff #208488)

I think it would be better to call this a reference. It's not really notes, and it's not just diagnostics.

thegameg updated this revision to Diff 208797.Jul 9 2019, 1:33 PM
thegameg marked 12 inline comments as done.

Address Jessica's comments. Thanks!

paquette accepted this revision.Jul 9 2019, 2:10 PM

LGTM!

This revision is now accepted and ready to land.Jul 9 2019, 2:10 PM
This revision was automatically updated to reflect the committed changes.