This is an archive of the discontinued LLVM Phabricator instance.

[OptDiag] Updating Remarks in SampleProfile
ClosedPublic

Authored by efriedma on Jul 31 2017, 5:50 PM.

Details

Summary

Updating remark API to newer OptimizationDiagnosticInfo API. This allows remarks to show up in diagnostic yaml file, and enables use of opt-viewer tool.

Hotness information for remarks (L505 and L751) do not display hotness information, most likely due to profile information not being propagated yet. Unsure if this is the desired outcome.

Diff Detail

Repository
rL LLVM

Event Timeline

tarunr created this revision.Jul 31 2017, 5:50 PM
anemet edited edge metadata.

Thanks very much for working on this! Please assign https://bugs.llvm.org/show_bug.cgi?id=33794 to you. Also check out my note in the bug.

Hotness information for remarks (L505 and L751) do not display hotness information, most likely due to profile information not being propagated yet. Unsure if this is the desired outcome.

You need to use -pass-remarks-with-hotness for that.

lib/Transforms/IPO/SampleProfile.cpp
163–183 ↗(On Diff #109039)

Rather than passing and ORE reference around like this, can't we make it a member to this class?

505 ↗(On Diff #109039)

Should these be OptimizationRemarkAnalysis instead? These aren't really optimization performed (or missed) i.e. either negative or positive.

506 ↗(On Diff #109039)

You probably want to stream this out as named-value argument. Looks for NV in Inliner.cpp for an example.

507–508 ↗(On Diff #109039)

I don't know much about the internals of sample-based profiling but is LineOffset or Discriminator actionable to the user? I.e. isn't this just a debugging aid if something goes wrong?

679–682 ↗(On Diff #109039)

Formatting only?

751–753 ↗(On Diff #109039)

Again see Inliner.cpp how to stream these out as named-value arguments.

1259–1265 ↗(On Diff #109039)

You can stream out the instruction like we stream them out I think in LICM as a named-value argument. This may include the LLVM IR name of the instruction which may or may not work in this case, so you may need to tweak DiagnosticInfoOptimizationBase::Argument to do the right thing here.

1492 ↗(On Diff #109039)

For the new pass manager path (when you enter via SampleProfileLoaderPass::run we shouldn't create ORE on the file but rather get it from FunctionAnalysisManagerModuleProxy. See IndirectCallPromotion.cpp for an example.

1503–1504 ↗(On Diff #109039)

Commit formatting change in otherwise unmodified code separately.

test/Transforms/SampleProfile/remarks.ll
30–40 ↗(On Diff #109039)

Please check the full records with YAML-NEXT.

tarunr added a comment.Aug 9 2017, 4:30 PM

Thanks for reviewing and for pointing me to examples.

-pass-remarks-with-hotness doesn't seem to be helping me in getting hotness information in these remarks.

lib/Transforms/IPO/SampleProfile.cpp
505 ↗(On Diff #109039)

Yes, I think that this makes sense

507–508 ↗(On Diff #109039)

The LineOffset and Discriminator may be useful for the user to better determine how the compiler applied the profile to the source. Though, I agree it doesn't seem very actionable to the user

1259–1265 ↗(On Diff #109039)

Currently it looks like the opcode of the instruction gets streamed out in DiagnosticInfoOptimizationBase::Argument. Do you mean that the original source code of the instruction or location of the instruction should be streamed out instead?

tarunr updated this revision to Diff 110496.Aug 9 2017, 4:34 PM

This update makes OptimizationRemarkEmitter part of the class, adds ORE to the promoteIndirectCall, and streams out remark messages as name-valued arguments.

tarunr marked 7 inline comments as done.Aug 9 2017, 4:41 PM
anemet added a comment.Aug 9 2017, 9:02 PM

-pass-remarks-with-hotness doesn't seem to be helping me in getting hotness information in these remarks.

It does something (after applying your patch):

./bin/opt -sample-profile -sample-profile-file=../test/Transforms/SampleProfile/Inputs/remarks.prof -S -pass-remarks-output=- -pass-remarks-with-hotness ../test/Transforms/SampleProfile/remarks.ll -o /dev/null | grep Hotness
Hotness: 0
Hotness: 0

but the 0 values are probably wrong. You probably need to study/debug this. The hotness should come from BFI which sample profiler should be updating.

lib/Transforms/IPO/SampleProfile.cpp
254 ↗(On Diff #110496)

Comments end in period.

730 ↗(On Diff #110496)

I think that this last use of the function that didn't use to pass ORE. Please remove the default argument from the declaration.

750 ↗(On Diff #110496)

We can't pass I here because it's remove by the time we get here? If yes, please comment.

1262–1264 ↗(On Diff #110496)

I think that the way we should handle this is like: << NV("CondBranchesLoc", BranchLoc)

And then add a new ctor Arguments(StringRef Key, DebugLoc DL) where you set Val to DL.getFileName() + DL.getLine() + DL.getCol() and also Loc to DL. I think this will appear correctly both on the compiler output and in YAML/HTML.

If you want to make that this works correctly in YAML/HTML, you can run opt-viewer on the YAML file. You will have to split out the original C source from the comment into a source file specified in the debug info.

1269 ↗(On Diff #110496)

Just stream out a DebugLoc() in you can't find. Don't the change the name of the field.

1449 ↗(On Diff #110496)

Default argument on the definition? Please remove.

751–753 ↗(On Diff #109039)

You didn't address this. See the Inliner or the Argument ctors; you can just stream out the Function itself.

-pass-remarks-with-hotness doesn't seem to be helping me in getting hotness information in these remarks.

It does something (after applying your patch):

./bin/opt -sample-profile -sample-profile-file=../test/Transforms/SampleProfile/Inputs/remarks.prof -S -pass-remarks-output=- -pass-remarks-with-hotness ../test/Transforms/SampleProfile/remarks.ll -o /dev/null | grep Hotness
Hotness: 0
Hotness: 0

but the 0 values are probably wrong. You probably need to study/debug this. The hotness should come from BFI which sample profiler should be updating.

Actually thinking a bit more about this, I don't think we can expect correct profile counts in BFI at this point. This pass adds the necessary metadata to get profile count on a subsequent run of BFI. @davidxl, am I right about this?

So I think for now we should just add a comment about this.

efriedma commandeered this revision.Aug 10 2017, 1:56 PM
efriedma added a reviewer: tarunr.
efriedma added a subscriber: efriedma.

(Tarun won't have time to finish this, so I'm taking it over.)

efriedma updated this revision to Diff 110639.Aug 10 2017, 3:24 PM
efriedma marked 10 inline comments as done.

Address review comments.

efriedma marked 3 inline comments as done.Aug 10 2017, 3:25 PM
anemet added inline comments.Aug 10 2017, 5:02 PM
lib/Transforms/IPO/SampleProfile.cpp
1260 ↗(On Diff #110639)

Why do you output the instruction? Its location should already be carried by remark location. It's also weird to see the instruction name in the remark:

CHECK: remark: nolocinfo.c:3:5: ret is the most popular destination for conditional branches at <UNKNOWN LOCATION>

efriedma updated this revision to Diff 110664.Aug 10 2017, 6:38 PM

Don't emit instruction names for "most popular destination" remarks.

anemet accepted this revision.Aug 10 2017, 7:45 PM

LGTM, thanks!

This revision is now accepted and ready to land.Aug 10 2017, 7:45 PM
This revision was automatically updated to reflect the committed changes.