Page MenuHomePhabricator

[OptRemarks] Make OptRemarks more generic: rename OptRemarks to Remarks
ClosedPublic

Authored by thegameg on Feb 21 2019, 5:23 PM.

Details

Summary

Getting rid of the name "optimization remarks" for anything that involves handling remarks on the client side.

It's safer to do this now, before we get stuck with that name in all the APIs and public interfaces we decide to export to users in the future.

This renames llvm/tools/opt-remarks to llvm/tools/remarks-shlib, and now generates libRemarks.dylib instead of libOptRemarks.dylib.

Diff Detail

Repository
rL LLVM

Event Timeline

thegameg created this revision.Feb 21 2019, 5:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2019, 5:23 PM
fhahn added inline comments.Feb 22 2019, 3:06 AM
clang/include/clang/Basic/CodeGenOptions.h
238 ↗(On Diff #187891)

Maybe RemarkRecordFile or something? Just RecordFile seems a bit unspecific to me. Also the comment above still refers to optimization records.

245 ↗(On Diff #187891)

Drop Optimization here?

clang/lib/CodeGen/CodeGenAction.cpp
268 ↗(On Diff #187891)

Same as above, maybe RemarkRecordFile?

clang/lib/Driver/ToolChains/Darwin.cpp
463 ↗(On Diff #187891)

Still refers to optimization record?

clang/lib/Frontend/CompilerInvocation.cpp
1213 ↗(On Diff #187891)

option still named OPT_opt_record_file.

JDevlieghere added inline comments.Feb 22 2019, 10:49 AM
clang/include/clang/Basic/CodeGenOptions.h
237 ↗(On Diff #187891)

Should this comment be updated as well?

238 ↗(On Diff #187891)

Or maybe RemarkFile for consistency with RemarkPattern below.

llvm/include/llvm-c/Remarks.h
34 ↗(On Diff #187891)

Are you sure this has no users? It's my understanding that we cannot break APIs in the C interface.

paquette added inline comments.Feb 22 2019, 10:50 AM
llvm/unittests/Remarks/RemarksParsingTest.cpp
278 ↗(On Diff #187891)

What happens if you give a negative line or column number?

What is the motivation of the rename? What are the plans for more generic OptRemarks?

thegameg updated this revision to Diff 188268.Feb 25 2019, 4:10 PM
thegameg marked 10 inline comments as done.
thegameg edited the summary of this revision. (Show Details)

I decided to change the goal for this patch a little:

  • Keep "optimization remarks" in the remark generation code. This makes more sense because it is specific to remarks for optimizations.
  • Only rename it in the parser/library code. This part of the code is going to be used on the client side (llvm-opt-report, opt-viewer, etc.), and this is the main part that we're looking to share for all the types of remarks.
llvm/include/llvm-c/Remarks.h
34 ↗(On Diff #187891)

This was never announced anywhere, so I believe it should be fairly safe to do the changes. @anemet, what do you think?

If this is not acceptable I'm happy to provide a compatibility layer.

llvm/unittests/Remarks/RemarksParsingTest.cpp
278 ↗(On Diff #187891)

For now, we get:

YAML:4:46: error: expected a value of integer type.

DebugLoc:        { File: test_parse.c, Line: -12, Column: 12 }
                                             ^~~

Which is pretty bad. I will update the parser in a future patch.

What is the motivation of the rename? What are the plans for more generic OptRemarks?

There are potential use cases where frontends or other tools could generate remarks, for things like "understanding" how the compiler treats some piece of code. This fits more in the "compiler logging" diagnostic which is exactly what remarks are today for optimizations.

thegameg marked an inline comment as done.Feb 25 2019, 4:15 PM
thegameg added inline comments.
llvm/unittests/Remarks/RemarksParsingTest.cpp
278 ↗(On Diff #187891)

(btw, the contents of this test didn't change, Phabricator just doesn't know that it was llvm/lib/OptRemarks/OptRemarksParser.cpp before)

This revision is now accepted and ready to land.Mar 4 2019, 5:39 PM
This revision was automatically updated to reflect the committed changes.