This is an archive of the discontinued LLVM Phabricator instance.

OpenMPOpt Remarks Support
ClosedPublic

Authored by jhuber6 on May 4 2020, 12:44 PM.

Details

Summary

Adding initial support for emitting remarks inside of OpenMPOpt. Added a Callback function to get an OptimizationRemarkEmitter object to the OpenMPOpt struct. The callback is used by the emitRemark function. Added two locations that emit simple remarks and a test file to verify functionality.

Diff Detail

Event Timeline

jhuber6 created this revision.May 4 2020, 12:44 PM

Looks pretty good. Some minor comments

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
560

Nit: getFunction()

auto & or auto *, please.

Do you need a default for the callback type?

613

I don't think it's a good idea to move the result here. Just return a reference.

667

Call getAnalysis here?

jhuber6 marked 3 inline comments as done.May 4 2020, 7:01 PM
jhuber6 added inline comments.
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
560

I changed the OREGetter to return a reference. I'm not exactly sure what you mean by default for the callback type. Do you mean a default implementation or are you talking about just the template parameter.

613

Yeah, I did this initially because I was having issues getting both the legacy and new versions to return a reference without memory leaks so I just did it by returning a unique_ptr. The std::move just cast it to an rvalue to avoid using the deleted constructor in the ORE class. I found another file that did something similar that had a single unique_ptr object to an ORE that's captured by reference. Then you just return a reference to that, so I'll change it to use that method because it's probably much more efficient.

667

I tried using the getAnalysis method but it caused it to seg-fault. I'm not exactly confident why but some other passes generate the ORE this way so I figured it was okay.

jhuber6 updated this revision to Diff 261989.May 4 2020, 7:14 PM

Changed OREGetter to return the ORE by reference rather than with a unique_ptr.

Please diff against the base version, e.g., by squashing all your commits into a single one.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
560

The latter. Doesn't this work:

template <typename RemarkKind, typename RemarkCallBack>
667

Oh, right. Yeah, so, the old PM CGSCC pass is bad at requiring function passes. Long story short, it doesn't work.

We should do what the inliner does, create an ORE whenever we need one for a function. Since we would also like hotness information in the new pass manager (which I think we should get automatically via the pass), we should not always create a new ORE but only in the old PM. That said, the ORE constructor says it can be expensive if hotness filtering is requested as block frequency is computed. Thus, in the old PM we can cache the OREs ourselves. So, we keep a map from function to ORE and in the OREGetter we lookup the ORE, if not there create it, and return it. WDYT?

jhuber6 marked 2 inline comments as done.May 4 2020, 8:42 PM
jhuber6 added inline comments.
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
560

I was just using that template to rename the type inside the function. I was pretty much just using it as this:

using RemarkCallBack = function_ref<RemarkKind(RemarkKind &&)>>;

Should I change that?

667

I was thinking about adding a cache previously since the function being worked on might not change often. It might be somewhat difficult to bake that functionality into the callback but I'll see what I can do in the morning.

jhuber6 updated this revision to Diff 262137.May 5 2020, 9:16 AM

I added a DenseMap to cache the Functions for the legacy PM's getter function. Let me know if I should've used a different data structure for this.

jhuber6 marked an inline comment as done.May 5 2020, 9:21 AM
jhuber6 added inline comments.
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
667

I just noticed I could probably have used try_emplace() rather than checking and inserting manually.

Last round of comments. After we should be good to go.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
29

Nit: probably not needed.

60

Just copy the getter (=function_ref)

560

I don't get the last comment but I guess we can just leave it as is.

671

I usually prefer:

std::make_unique<OptimizationRemarkEmitter> &ORE = OREMap[F];
if(!ORE)
  ORE = ...
return ORE;

Your code has 3 lookups into the map, mine only one.

jhuber6 updated this revision to Diff 262224.May 5 2020, 2:20 PM

Making suggested changes.

jdoerfert accepted this revision.May 5 2020, 3:08 PM

LGTM. I try to commit it later.

This revision is now accepted and ready to land.May 5 2020, 3:08 PM
hfinkel added a subscriber: hfinkel.May 6 2020, 4:05 PM
hfinkel added inline comments.
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
336

Why doesn't RFI.Name have a ore::NV field name. Tools might certainly be interested in what functions are being optimized.

370

Same here: a NV for RFI.Name seems useful.

jhuber6 updated this revision to Diff 262714.May 7 2020, 11:57 AM

Adding a remark and test for parallel region deletion. Added a NV tag for the runtime function deduplicated / moved.

Just realized the master branch changed the attributor flags for the test file. Should I add a diff that's more up-to-date?

jhuber6 updated this revision to Diff 263311.May 11 2020, 4:56 PM

Updating files closer to origin.

This revision was automatically updated to reflect the committed changes.