This is an archive of the discontinued LLVM Phabricator instance.

Make new PM honor -fdebug-info-for-profiling
ClosedPublic

Authored by danielcdh on Jul 21 2017, 3:53 PM.

Details

Summary

The new PM needs to invoke add-discriminator pass when building with -fdebug-info-for-profiling.

Event Timeline

danielcdh created this revision.Jul 21 2017, 3:53 PM

Chandler, could you help suggest how I could add unittest for this change? I was trying to find similar test for PGOOpt, but could not find any tests that's related.

Thanks,
Dehao

chandlerc added inline comments.Jul 21 2017, 3:58 PM
include/llvm/Passes/PassBuilder.h
47

(If we end up using this flag, see below) Maybe call this "AddDebugInfoDiscriminators"? or otherwise make it more clear what changes it introduces?

lib/Passes/PassBuilder.cpp
333–334

Does this need to be at this *exact* position? Could it be done before all passes and directly added to the pipeline by the frontend?

Alternatively, could you use one of the extension points we have in the new PM for this?

If not, I would suggest that we need an opt flag that sets this and test it that way.

Yet another alternative: what about keying this off of the existing PGO options rather than a separate flag? (Side note: do we have a way to set up the PGO options using the opt tool?)

danielcdh added inline comments.Jul 21 2017, 4:24 PM
lib/Passes/PassBuilder.cpp
333–334

This does not need to be at the exact position. It should be done before all passes. I'm not sure how to directly add this to the pipeline by FE, looks like all the passes added by FE are all module passes. As AddDiscriminatorsPass is a function pass, I'm not sure how to add it to the MPM. Could you suggest how to do that in new PM?

Could you point me to an example of how to use extension points in the new PM?

We could add this to PGOOpt if you think that is a good option. I don't think that we could pass in the PGOOpt to the opt tool.

Thanks,
Dehao

chandlerc added inline comments.Jul 21 2017, 4:38 PM
lib/Passes/PassBuilder.cpp
333–334

The more I think about it, the more I think adding this pass should be triggered much like the other profiling-specific passes are triggered from PGOOpt.

And I really do think we want a way to setup and pass in PGOOpt from the opt commandline. What seems challenging there?

danielcdh updated this revision to Diff 108171.Jul 25 2017, 3:39 PM

update the option to put it within PGOOPtions. Will add test once https://reviews.llvm.org/D35807 is committed.

chandlerc added inline comments.Jul 25 2017, 6:09 PM
include/llvm/Passes/PassBuilder.h
35

Calling this profile generation seems odd.

I would call this more along the lines of supporting sample PGO *at all* (even if not using a specific profile file), and then set it to true if a file is provided (which will become more obvious with a constructor when rebased on your other patch).

Then the condition below seems simpler.

danielcdh added inline comments.Jul 25 2017, 9:25 PM
include/llvm/Passes/PassBuilder.h
35

Updated the patch to change the name, also added tests.

Not sure what name to use at the opt driver, any suggestions?

Thanks,
Dehao

danielcdh updated this revision to Diff 108212.Jul 25 2017, 9:25 PM

update flag name and add test.

chandlerc added inline comments.Jul 25 2017, 9:33 PM
tools/opt/NewPMDriver.cpp
95–97 ↗(On Diff #108212)

Generating more PGO-friendly debug information seems orthogonal to the others (but perhaps reasonable to default "on" when *using* a sample profile), so I would leave it as a separate flag entirely.

I don't have great naming ideas... I guess steal from the Clang name: -new-pm-debug-info-for-profiling.

danielcdh marked an inline comment as done.

update

This revision is now accepted and ready to land.Jul 25 2017, 10:39 PM
danielcdh closed this revision.Jul 26 2017, 8:01 AM
modocache added a subscriber: modocache.EditedJul 26 2017, 8:36 AM

Just FYI, test/Other/new-pm-pgo.ll appears to be failing on several of the buildbots. One example: http://lab.llvm.org:8011/builders/clang-hexagon-elf/builds/10801

Just FYI, test/Other/new-pm-pgo.ll appears to be failing on several of the buildbots. One example: http://lab.llvm.org:8011/builders/clang-hexagon-elf/builds/10801

Thanks for the heads up.

I only tested this on release compiler, thus assertion failure is not triggered. Sorry about that.

Committed r309125 as NFC to fix the issue. Please let me know if you think that I should revert both changes and send a new one for review.