Page MenuHomePhabricator

[PM] Add support for instrumented PGO in the new pass manager (clang-side)

Authored by davide on Jan 30 2017, 2:40 PM.



This implements the clang bits of
I'm planning to use this to test the PGO inliner changes available only in the new pass manager.
I'm not very familiar with the clang driver so I'm not sure if this change is correct. PTAL (also suggestions on how to best test this are welcome).

Diff Detail


Event Timeline

davide created this revision.Jan 30 2017, 2:40 PM
davidxl added inline comments.Jan 31 2017, 11:05 AM
805 ↗(On Diff #86353)

can you define a macro for the default profile file name to be shared?

davide added inline comments.Jan 31 2017, 11:08 AM
805 ↗(On Diff #86353)

Sure thing. I generally prefer a global const std::string (trying to avoid macros), is that the same to you?

Thanks. I'll update a new version shortly. The llvm side of the patch is slightly more interesting as I made some assumptions (and I still am learning about PGO so I'm not entirely sure I put in the correct place in the pipeline)

chandlerc added inline comments.Feb 8 2017, 10:40 AM
805 ↗(On Diff #86353)

Global std::string objects are problematic due to the destructor.

But we just recently added a handy string literal type to LLVM. =]

davide updated this revision to Diff 87713.Feb 8 2017, 2:25 PM

Addressed comments. Thanks!

davide added inline comments.
63–65 ↗(On Diff #87713)

FWIW, this was part of AssemblyHelper initially.
I thought that worked, instead I got a linker error (undefined reference to etc..)
I can't claim to be a C++ expert so I asked @Bigcheese and he pointed out this could actually be a bug in clang.

davide added inline comments.Feb 10 2017, 11:28 AM
63–65 ↗(On Diff #87713)

Just for the records, not a clang bug, it's missing an out-of-line def (thanks zygoloid for pointing out)

chandlerc edited edge metadata.Feb 10 2017, 11:28 AM

FWIW, this all looks good on my end. David, does this address your concern?

63–65 ↗(On Diff #87713)

As discussed in IRC, looks like we know how to sink this into the class now.

This revision was automatically updated to reflect the committed changes.