This is an archive of the discontinued LLVM Phabricator instance.

[OptRemarks] Add library for parsing optimization remarks
ClosedPublic

Authored by thegameg on Oct 2 2018, 3:22 AM.

Details

Summary

Add a library that parses optimization remarks (currently YAML, so based on the YAMLParser).

The goal is to be able to provide a remark parser that is not completely dependent on YAML, in case we decide to change the format later.

It exposes a C API which takes a handler that is called with the remark structure. It adds a libLLVMOptRemark.a static library, and it's used in-tree by the llvm-opt-report tool (from which the parser has been mostly moved out).

The plan is to also migrate the opt-viewer to use this interface by providing python bindings or such.

Diff Detail

Repository
rL LLVM

Event Timeline

thegameg created this revision.Oct 2 2018, 3:22 AM
JDevlieghere added inline comments.Oct 2 2018, 5:52 AM
include/llvm-c/OptRemarks.h
84 ↗(On Diff #167910)

The inconsistency in the comments bothers me. Sometimes the key is mentioned, followed by a colon or a period, sometimes it's the value, possibly between single quotes. How about putting both between quotes? The grouping is also not clear to me.

124 ↗(On Diff #167910)

What happens if you call this after you encounter an error?

146 ↗(On Diff #167910)

s/on/of/

lib/OptRemarks/OptRemarksParser.cpp
24 ↗(On Diff #167910)

I usually use doxygen style comments even in lib, in case it ever gets moved to a header.

78 ↗(On Diff #167910)

Why don't you return the StringRef? Can the function return false and a non-empty string / valid optional? If so that'd be a good thing to mention here. Same for the parse functions below.

143 ↗(On Diff #167910)

s/old/Old/, same in the functions below.

239 ↗(On Diff #167910)

Maybe extract a convenience method for constructing a LLVMOptRemarkStringRef from a StringRef?

303 ↗(On Diff #167910)

So true means there was an error and false means success? That's probably worth mentioning in a comment as well. There was a discussion about this on the mailing list, but I'm not sure if anything was decided yet.

thegameg updated this revision to Diff 167954.Oct 2 2018, 8:31 AM
thegameg marked 6 inline comments as done.
thegameg edited the summary of this revision. (Show Details)

Thanks Jonas for the review.

  • Unify the comments format.
  • Use Doxygen in /lib as well.
  • Add conversion function from StringRef to LLVMOptRemarkStringRef.
  • Return true on success and false otherwise.
  • Fix naming conventions.
  • Immediately return nullptr if the parser had any errors.
  • Check for *exactly* one remark in the unit tests. This also tests for the return value of GetNext.

Mostly style nits here.

Biggest thing is wondering if we can use llvm::Error here instead of the error and errorAndSkip functions?

include/llvm-c/OptRemarks.h
110 ↗(On Diff #167954)

How about "\p Buf cannot be NULL"?

123 ↗(On Diff #167954)

pointed to by

126 ↗(On Diff #167954)

Simpler:

If the parser reaches the end of the buffer, the return value will be NULL.

131–132 ↗(On Diff #167954)

Space between list items for consistency with following list. e.g

  1. x
  1. y
lib/OptRemarks/OptRemarksParser.cpp
28 ↗(On Diff #167954)

Remove newline for consistency?

34 ↗(On Diff #167954)

s/di/DI/

44 ↗(On Diff #167954)

Skip shouldn't be capitalized

45 ↗(On Diff #167954)

Should this comment be above Tag?

90 ↗(On Diff #167954)

Can we use llvm::Error for this and the other error handling stuff here?

206 ↗(On Diff #167954)

I think that the first letter in these error messages should be capitalized

252 ↗(On Diff #167954)

Why are we returning !OldSkipThisOne? A comment would be good here.

313 ↗(On Diff #167954)

Skip doesn't need to be capitalized

thegameg updated this revision to Diff 168106.Oct 3 2018, 6:03 AM
thegameg marked 10 inline comments as done.

Thanks Jessica for the review.

  • Addressed comments.
  • Removed skipping logic.
  • Use llvm::Error. Much much better! Thanks!
  • Update tests.
thegameg added inline comments.Oct 3 2018, 6:04 AM
include/llvm-c/OptRemarks.h
84 ↗(On Diff #167910)

The grouping is mostly mandatory vs optional. Are you suggesting something else?

124 ↗(On Diff #167910)

I added an extra check.

lib/OptRemarks/OptRemarksParser.cpp
78 ↗(On Diff #167910)

Mostly for consistency with parseDebugLoc and parseArg. Another nice thing is that I can write things like:

parseValue(Str, Node) && errorAndSkip("error message here", Node)
303 ↗(On Diff #167910)

Fair point, thanks! It seems that the discussion was going more towards returning true on success. I'll update everything accordingly.

44 ↗(On Diff #167954)

Actually, the comment is irrelevant now. Removed.

45 ↗(On Diff #167954)

Tag was too confusing. I removed it and called it Type.

206 ↗(On Diff #167954)

I would like to, but they show up like this in the diagnostics:

error: YAML:2:1: error: key is not a string.

{ a: b }:            inline
^

and clang follows this as well.

252 ↗(On Diff #167954)

Removed as part of the error refactoring.

thegameg added inline comments.Oct 3 2018, 6:05 AM
lib/OptRemarks/OptRemarksParser.cpp
303 ↗(On Diff #167910)

Forget that, I updated it to llvm::Error instead.

JDevlieghere added inline comments.Oct 3 2018, 6:33 AM
include/llvm-c/OptRemarks.h
84 ↗(On Diff #167910)

No, it's just that it wasn't clear to me. Maybe remove the newlines between DebugLoc and Hotness, and Hotness and Args, to mirror the grouping for the mandatory fields.

lib/OptRemarks/OptRemarksParser.cpp
78 ↗(On Diff #167910)

My comment was more general now that you're using Error you could return an Expected. Anyway, it's probably not worth is you'd need an extra struct for parseDebugLoc and you'd loose the nice thing you mentioned.

119 ↗(On Diff #168106)

How about something less generic like toOptRemarkStr?

thegameg updated this revision to Diff 168117.Oct 3 2018, 8:18 AM
thegameg marked 5 inline comments as done.

Address Jonas' comments.

thegameg updated this revision to Diff 168120.Oct 3 2018, 8:51 AM

Fix some spacing issues.

Just one more round of comments.

Other than that, I think that this is good.

lib/OptRemarks/OptRemarksParser.cpp
47–48 ↗(On Diff #168120)

If File is optional, then why not make it an Optional<StringRef> to make it clear?

54 ↗(On Diff #168120)

Is this comment out of date? I don't see TmpArgs referenced anywhere in here.

91–92 ↗(On Diff #168120)

Is Parser (and I guess Ctx) guaranteed to not be a nullptr? If not, I can see Parser->ErrorStream causing some issues.

135 ↗(On Diff #168120)

Should this an similar occurrences be "expected" rather than "expecting"? Errors are usually past-tense.

234–235 ↗(On Diff #168120)

Maybe split this into two errors to make it clear which one is missing?

thegameg updated this revision to Diff 168448.Oct 5 2018, 3:23 AM
thegameg marked 5 inline comments as done.

Address Jessica's latest comments.

thegameg updated this revision to Diff 168451.Oct 5 2018, 3:41 AM

And...update the tests.

thegameg added inline comments.Oct 8 2018, 5:20 AM
lib/OptRemarks/OptRemarksParser.cpp
91–92 ↗(On Diff #168120)

I added an assert. It should never be null since it's passed around by the SourceMgr. It's set by doing:

SM.setDiagHandler(RemarkParser::HandleDiagnostic, this);

previously.

thegameg updated this revision to Diff 168645.Oct 8 2018, 5:21 AM
  • Remove some out-of-date comments.

Is there anything left to discuss or is this ready to land?

paquette accepted this revision.Oct 10 2018, 8:58 AM

LGTM!

This revision is now accepted and ready to land.Oct 10 2018, 8:58 AM
This revision was automatically updated to reflect the committed changes.