This is an archive of the discontinued LLVM Phabricator instance.

[Remarks] Add a new Remark / RemarkParser abstraction
ClosedPublic

Authored by thegameg on Mar 6 2019, 1:52 PM.

Details

Summary

This adds a Remark class that allows us to share code when working with remarks.

The C API has been updated to reflect this. Instead of the parser generating C structs, it's now using a C++ object that is used through opaque pointers in C. This gives us much more flexibility on what changes we can make to the internal state of the object and interacts much better with scenarios where the library is used through dlopen.

  • C API updates:
    • move from C structs to opaque pointers and functions
    • the remark type is now an enum instead of a string
  • unit tests updates:
    • use mostly the C++ API
    • keep one test for the C API
    • rename to YAMLRemarksParsingTest
  • a typo was fixed: AnalysisFPCompute -> AnalysisFPCommute.
  • a new error message was added: "expected a remark tag."
  • llvm-opt-report has been updated to use the C++ parser instead of the C API

Diff Detail

Event Timeline

thegameg created this revision.Mar 6 2019, 1:52 PM
Herald added a project: Restricted Project. · View Herald Transcript

Some inline comments, overall this looks good but I'll need to do another pass to make sure I understand everything correctly :-)

llvm/include/llvm/Remarks/Remark.h
66

This looks a little odd, any chance you could give the variable a different name?

llvm/include/llvm/Remarks/RemarkParser.h
37

I don't fully understand how this enforces anything?

llvm/lib/Remarks/RemarkParser.cpp
86

early return?

llvm/lib/Remarks/YAMLRemarkParser.cpp
131

Can we extract a function from the loop body?

183

Same here, can we simplify this by extracting a function?

llvm/lib/Remarks/YAMLRemarkParser.h
47

Does this have to be a pointer? Can it be a reference?

One a more general level: why does the ParseState not own the buffer? Are you reducing the number of allocations? Maybe worth adding a comment with the reason why.

115

DI outside the context of this class is really hard to follow. Consider adding a getter or giving this a more meaningful name.

117

nit: had or has?

thegameg updated this revision to Diff 189623.Mar 6 2019, 5:11 PM
thegameg marked 10 inline comments as done.

Thanks Jonas! Addressed all the commends.

llvm/include/llvm/Remarks/RemarkParser.h
37

Right, it doesn't!

llvm/lib/Remarks/YAMLRemarkParser.h
47

I had issues with assigning a new state, but I just discovered Optional<T>::emplace. Thanks! Added the reason for the buffer re-use.

JDevlieghere added inline comments.Mar 12 2019, 12:30 PM
llvm/include/llvm/Remarks/RemarkParser.h
37

So why is it there? :-)

llvm/lib/Remarks/RemarkParser.cpp
86

I would've done it the other way around, with early returns for everything that can go wrong first. I think that could save you another level of indentation.

llvm/lib/Remarks/YAMLRemarkParser.cpp
25
if(auto* Key = ....) { 
  Result = Key->getRawValue();
  return Error::success();
}
return make_error<YAMLParseError>("key is not a string.", Node);
thegameg updated this revision to Diff 190368.Mar 12 2019, 6:05 PM
thegameg marked 3 inline comments as done.

Address Jonas' comments.

This revision is now accepted and ready to land.Mar 19 2019, 9:27 AM
This revision was automatically updated to reflect the committed changes.
rupprecht added inline comments.
llvm/trunk/lib/Remarks/RemarkParser.cpp
23 ↗(On Diff #191357)

Because ParserImpl doesn't have a virtual destructor, ~ParserImpl() is called instead of ~YAMLParserImpl() in ~Parser(). (At least that's my understanding of the error).

We noticed these failures when running YAMLRemarksParsingTest with asan. It should be fixed by rL356583.

thegameg marked an inline comment as done.Mar 20 2019, 11:14 AM
thegameg added inline comments.
llvm/trunk/lib/Remarks/RemarkParser.cpp
23 ↗(On Diff #191357)

Good catch, thank you for fixing it!

llvm/tools/remarks-shlib/Remarks.exports