Page MenuHomePhabricator

Attach profile summary information to module

Authored by eraman on Mar 18 2016, 4:14 PM.



This allows optimization passes to make use of detailed profile summary. Once this is in and the optimization passes are made to use this, the "MaxFunctionCount" flag will be removed as the profile summary supersedes that.

Diff Detail


Event Timeline

eraman updated this revision to Diff 51088.Mar 18 2016, 4:14 PM
eraman retitled this revision from to Attach profile summary information to module.
eraman updated this object.
eraman added a reviewer: vsk.
eraman added subscribers: cfe-commits, davidxl.
vsk edited edge metadata.Mar 19 2016, 4:12 PM

Thanks! Some inline comments --

399 ↗(On Diff #51088)

getModule().getContext() -> VMContext?

401 ↗(On Diff #51088)

I don't think we need this assert. A lot of code depends on MDTuple::get just working -- it's fine to crash if it doesn't.

5 ↗(On Diff #51088)

ISTM that a lot of the lines in this file do not contribute additional coverage of the changes introduced by your patch.

We already have tests which show that the summary code works (general.proftext). I think we just need 1 empty function with 1 counter in this file. We don't need to check that the entire detailed summary appears in the metadata, just one cutoff entry should suffice.

davidxl added inline comments.Mar 22 2016, 2:21 PM
398 ↗(On Diff #51088)

This line will be removed later once the client code has been updated right?

399 ↗(On Diff #51088)

or Module &M = getModule();
M is also referenced in previous line

5 ↗(On Diff #51088)

That test is actually not as good for testing summary computation as this one. Perhaps we can simplify this test and move this test (removing the clang dump part) into llvm-profdata dir.

vsk added inline comments.Mar 22 2016, 2:24 PM
5 ↗(On Diff #51088)

Makes sense to me.

eraman updated this revision to Diff 51342.Mar 22 2016, 2:34 PM
eraman edited edge metadata.

Address Vedant's comments

eraman marked 2 inline comments as done.Mar 22 2016, 2:37 PM
eraman added inline comments.
398–399 ↗(On Diff #51342)

The previous line? Yes.

6 ↗(On Diff #51342)

I didn't see David's comment and your subseqquent response. I have changed the test to only check for the presence of Profilesummary and DetailedSummary fields.

Since test case tests this change - that profile summary is attached to the module - I think it is sufficient. David and Vedant, does this sound reasonable?

vsk accepted this revision.Mar 24 2016, 1:31 PM
vsk edited edge metadata.

This lgtm.

IMO we'd get the same coverage if the C file contained void foo() {}, but that's just a nit :). It'd be good to follow up with David about what he thinks is lacking in general.proftext, but that's for a later commit.

This revision is now accepted and ready to land.Mar 24 2016, 1:31 PM
davidxl accepted this revision.Mar 24 2016, 1:31 PM
davidxl added a reviewer: davidxl.


This revision was automatically updated to reflect the committed changes.