Page MenuHomePhabricator

Refine the PGOOpt and SamplePGOSupport handling.
ClosedPublic

Authored by danielcdh on Jul 28 2017, 5:32 PM.

Details

Summary

Now that SamplePGOSupport is part of PGOOpt, there are several places that need tweaking:

  1. AddDiscriminator pass should *not* be invoked at ThinLTOBackend (as it's already invoked in the PreLink phase)
  2. addPGOInstrPasses should only be invoked when either ProfileGenFile or ProfileUseFile is non-empty.
  3. SampleProfileLoaderPass should only be invoked when SampleProfileFile is non-empty.
  4. PGOIndirectCallPromotion should only be invoked in ProfileUse phase, or in ThinLTOBackend of SamplePGO.

Event Timeline

danielcdh created this revision.Jul 28 2017, 5:32 PM
chandlerc edited edge metadata.Jul 28 2017, 5:37 PM

I think this adds way too much complexity to these predicates. I think we need a much better approach here... Have you looked at alternatives? Is the technique being used to add the descriminators pass just the wrong technique given the interaction with ThinLTO

I think this adds way too much complexity to these predicates. I think we need a much better approach here... Have you looked at alternatives? Is the technique being used to add the descriminators pass just the wrong technique given the interaction with ThinLTO

This was how we handle AddDiscriminator pass in the legacy PM. I have not come up with a better alternative, could you help suggest? Thanks

Mostly see how to improve ThinLTO flags here. Need to understand better the other changes.

lib/Passes/PassBuilder.cpp
537–538

I'd suggest hoisting this into the caller, and its own module-to-function adaptor so we walk the entire module and add discriminators once befare we do anything else. That'll match the other similar "semantic" pass we have where we force certain function attributes.

574–576

I don't think this assert is adding much value and it makes things harder to read...

I would move this assert into maybe more precise asserts in the PGOOpt class itself.

Then I think a comment here explaining the logic would help. Before, it seemed obvious: if we're not doing sample profiling, add the instrumentation-based profiling passes. Otherwise, add the sample passes. Now the logic seems more complex....

586–588

Why do we want to do this *before* the thin link with instrumentation PGO but not with sample PGO?

danielcdh marked an inline comment as done.Jul 28 2017, 6:30 PM
danielcdh added inline comments.
lib/Passes/PassBuilder.cpp
574–576

Moved the assert to PGOOptions class.

Added the comment to make it more clear. I think the logic is still the same, the first if handles instrumentation based PGO, the 2nd handles sample based PGO.

586–588

As illustrated in the comment, we do not enable it in PrepareForThinLTO phase during sample PGO because it changes IR to makes profile annotation in backend compile inaccurate.

I'm not sure why ICP is invoked twice in both prelink and backend.

danielcdh updated this revision to Diff 108762.Jul 28 2017, 6:31 PM

Integrate Chandler's reviews. Thanks!

chandlerc added inline comments.Jul 28 2017, 6:48 PM
lib/Passes/PassBuilder.cpp
574–576

So, the logic hasn't actually changed? If not, maybe leave the original code? I found it a bit easier to read. Not a huge deal either way.

782–785

You don't actually need an FPM, you can directly hand the pass to the adaptor.

danielcdh updated this revision to Diff 108763.Jul 28 2017, 6:57 PM
danielcdh marked an inline comment as done.

update

danielcdh added inline comments.Jul 28 2017, 6:57 PM
lib/Passes/PassBuilder.cpp
574–576

Sorry, did not make this clear: the old logic was trying to do the same thing (handle instrPGO and samplePGO in the branch). But the original logic was wrong. It assumes that if SampleProfileFile is empty, then one of the InstrProfile flags will be non-empty. This assumption was broken when we introduced SamplePGOSupport. As all 3 file paths could be empty, while SamplePGOSupport==true.

chandlerc accepted this revision.Jul 28 2017, 7:03 PM

LGTM w/ tweak below.

lib/Passes/PassBuilder.cpp
586–588

I suspect we should follow-up on this and change sample PGO and instrumentation PGO to behave the same way here (or have a clear explanation of why they differ).

But that can happen later.

805–808

Same as above, can directly add the pass to the adaptor.

This revision is now accepted and ready to land.Jul 28 2017, 7:03 PM
danielcdh marked an inline comment as done.Jul 28 2017, 7:19 PM
danielcdh added inline comments.
lib/Passes/PassBuilder.cpp
586–588

Acknowledged.

Teresa and David, do you remember why we invoke ICP twice in instrumentation based FDO?

danielcdh closed this revision.Jul 28 2017, 9:10 PM