Page MenuHomePhabricator

[PGO] Add option to dot-dump raw profile counts as computed by profile annotator
ClosedPublic

Authored by davidxl on Jan 23 2017, 11:44 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

davidxl created this revision.Jan 23 2017, 11:44 AM
davide added a subscriber: davide.Jan 23 2017, 11:54 AM

This one is very useful. I guess we may want to add documentation for it (and maybe the other flag you just added?)?

I won't mind adding documentation of the flags. Is there a known place to do so (for internal options)?

I won't mind adding documentation of the flags. Is there a known place to do so (for internal options)?

I personally wouldn't mind them to be added to https://clang.llvm.org/docs/UsersManual.html#profile-guided-optimization (maybe in a separate subsection). It's generally nicer having these flags documented somewhere than looking at the code, IMHO

silvas added a subscriber: silvas.Jan 23 2017, 11:25 PM

I won't mind adding documentation of the flags. Is there a known place to do so (for internal options)?

I personally wouldn't mind them to be added to https://clang.llvm.org/docs/UsersManual.html#profile-guided-optimization (maybe in a separate subsection). It's generally nicer having these flags documented somewhere than looking at the code, IMHO

This is an internal flag. That page is end-user documentation. Officially, users are not allowed to use -mllvm flags and we should not encourage them to do so. I'm not aware of a good place to document this flag unfortunately.

xur edited edge metadata.Jan 26 2017, 1:20 PM

This gonna be very useful in debug PGO related issues -- we don't need to use the instrumentation debug dumps to track the counts (which is not available in debug builds).
Overall, the patch looks good to me except the minor issues (see inline comments).

lib/Transforms/Instrumentation/PGOInstrumentation.cpp
1240 ↗(On Diff #85439)

ViewGraph is interactive. Is WriteGraph better?
I tried the patch without specified the PGOViewFunction, it just pops too many windows at once.

1331 ↗(On Diff #85439)

Check if BI is null. If the Node is not reachable from entry. We don't have a UseBBInfo.

1332 ↗(On Diff #85439)

CountValid for the internal use. After count prorogation, all the reachable BBs should have a valid count.

davidxl updated this revision to Diff 85966.Jan 26 2017, 1:57 PM

Address review feedback from xur.

davidxl marked an inline comment as done.Jan 26 2017, 1:59 PM
davidxl added inline comments.
lib/Transforms/Instrumentation/PGOInstrumentation.cpp
1240 ↗(On Diff #85439)

make it display only there is one function specified. Also add function name in filenames.

1331 ↗(On Diff #85439)

Added check. How can that happen?

1332 ↗(On Diff #85439)

That is why unknown is displayed so that errors like this can be spotted.

xur accepted this revision.Jan 26 2017, 3:20 PM
xur added inline comments.
lib/Transforms/Instrumentation/PGOInstrumentation.cpp
1331 ↗(On Diff #85439)

We construct the Minimum spanning tree starting the entry node. An reachable BB (not clean up) will be left out without being touched. This is race, but does happen in some constructed .ll files.

This revision is now accepted and ready to land.Jan 26 2017, 3:20 PM
This revision was automatically updated to reflect the committed changes.
davidxl marked an inline comment as done.