Page MenuHomePhabricator

[PGO] Use SourceFileName rather module name in PGOFuncName
ClosedPublic

Authored by xur on Fri, Jan 4, 12:02 PM.

Details

Summary

In LTO or Thin-lto mode (though linker plugin), the module names are of temp file names which are different for different complations. Using SourceFileName avoids the issue.
This should not change any functionality for current PGO as all the current callers of getPGOFuncName() is before LTO.

Diff Detail

Repository
rL LLVM

Event Timeline

xur created this revision.Fri, Jan 4, 12:02 PM
davidxl added inline comments.Fri, Jan 4, 12:29 PM
lib/ProfileData/InstrProf.cpp
255 ↗(On Diff #180293)

When StaticFullModulePrefix is false, should the base name be used ?

xur marked an inline comment as done.Fri, Jan 4, 2:13 PM
xur added inline comments.
lib/ProfileData/InstrProf.cpp
255 ↗(On Diff #180293)

Yes. when it's false, only use the basename.
In this patch, when it's false, StripLevel is -1, in which case, we strip all the paths and leave only the basename.

There is a test
test/Transforms/PGOProfile/statics_counter_naming.ll
that tests the stripping behavior.

davidxl added inline comments.Fri, Jan 4, 2:17 PM
lib/ProfileData/InstrProf.cpp
255 ↗(On Diff #180293)

but the stripLevel will be reset to StaticFuncStripDirNamePrefix which is 0 by default?

xur marked an inline comment as done.Fri, Jan 4, 2:29 PM
xur added inline comments.
lib/ProfileData/InstrProf.cpp
255 ↗(On Diff #180293)

It will not be reset as the cond in line 257 will be false if StripLevel is (unsigned) -1.

davidxl accepted this revision.Fri, Jan 4, 2:34 PM

lgtm

lib/ProfileData/InstrProf.cpp
255 ↗(On Diff #180293)

ok, please put an explicit type casting before '-1' to make it clearer.

This revision is now accepted and ready to land.Fri, Jan 4, 2:34 PM
kristina requested changes to this revision.Mon, Jan 7, 8:52 PM
kristina added a subscriber: kristina.

I feel like this needs a test or amending an existing test, a bit too tired right now to patch this into my fork and run the test suite, but I feel as-is, this could cause test failures (and if it doesn't, that's likely indicative of missing coverage). Please address that before committing it. Also, see inline comments, first one is a style nit but I'm more concerned about uint32_t StripLevel = StaticFuncFullModulePrefix ? 0 : -1;.

lib/ProfileData/InstrProf.cpp
255 ↗(On Diff #180293)

What's the rationale behind changing the way FileName is constructed? I don't think there's a reason to change this from an assign-constructor (it's not consistent with surrounding code and even rest of the patch).

256 ↗(On Diff #180293)

I'm not exactly sure what is happening here. Why are you storing a signed value in an unsigned type? In addition, what's the rationale behind using an stdint type here? A simple int should suffice.

This revision now requires changes to proceed.Mon, Jan 7, 8:52 PM
xur marked 2 inline comments as done.Tue, Jan 8, 9:15 AM
xur added inline comments.
lib/ProfileData/InstrProf.cpp
255 ↗(On Diff #180293)

it's to set to the largest int. In the committed version, there is an explicit conversion.

255 ↗(On Diff #180293)

As I mentioned in the initial reviews, using source filename makes more sense than using module name.
There is not issue now as the callers are before LTO. If we call the function in or after LTO, the PGOFuncName will be different for profile-gen and profile-use pass.

Differential Review is mostly for pre-commit review, add in "Differential Revision: https://reviews.llvm.org/DXXXXX" to the commit message and it will automatically link the commit and diff and close it. As an alternative you can do that manually, It seems bigger patch has already been committed in rL350579 (without review) and this was pending review rL350442. I've linked those in but if you say it's been fixed in the committed version and this is a followup, can you rebase the patch on the current trunk? For post-commit reviews Audit may be a better place if you need that.

Differential Review is mostly for pre-commit review, add in "Differential Revision: https://reviews.llvm.org/DXXXXX" to the commit message and it will automatically link the commit and diff and close it. As an alternative you can do that manually, It seems bigger patch has already been committed in rL350579 (without review)

@xur - something weird happened with your commits. The one mentioned by @kristina above (r350579) was actually D56342 ([llvm-profdata] add value-cutoff functionality to show command) which was reviewed, but somehow got the same title and commit message as this patch. Not sure if there is a way to go back and fix that in the commit log...if not I'd recommend reverting it and recommitting with the correct commit log and title.

Also, as @kristina mentions, if your commit log contains a message like "Differential Revision: https://reviews.llvm.org/DXXXXX" it would link to the patch on phab.

and this was pending review rL350442. I've linked those in but if you say it's been fixed in the committed version and this is a followup, can you rebase the patch on the current trunk? For post-commit reviews Audit may be a better place if you need that.

This revision was not accepted when it landed; it landed in state Needs Revision.Tue, Jan 8, 2:44 PM
This revision was automatically updated to reflect the committed changes.

Beyond the weird commit message issue, there are also comments on this revision. They arrived after it was landed in the tree, so its more like post-commit review, but the comments should still be addressed or responded to here.

xur added a comment.Wed, Jan 9, 12:18 PM

@kristina: This patch does not functionally change anything in current code base. We think this patch makes more sense than the original one (which also contributed by us)
Current code works fine (as all the callers to getPGOFuncName() are before LTO). Tests for stripping behavior is already in the test suite.
It will break once that functions is called in LTO or after LTO with unknown function error. This will show in patch here https://reviews.llvm.org/D54175.

I cannot write a new test for this as there is currently no caller from LTO or passes after LTO.

If I cannot land this patch alone, I have to move it to D54175.