[PGO] Use SourceFileName rather module name in PGOFuncName

[xur]

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.

[davidxl]

lib/ProfileData/InstrProf.cpp

255 ↗

(On Diff #180293)

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

[xur]

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]

lib/ProfileData/InstrProf.cpp

255 ↗

(On Diff #180293)

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

[xur]

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]

lgtm

lib/ProfileData/InstrProf.cpp

255 ↗

(On Diff #180293)

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

[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.

[xur]

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.

[kristina]

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.

[tejohnson]

In D56327#1350006, @kristina wrote:

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.

[chandlerc]

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]

@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.

llvm/trunk/lib/ProfileData/InstrProf.cpp

 // used when computing the GUID that is compared against the profiled GUIDs.
 // To differentiate compiler generated internal symbols from original ones,
 // PGOFuncName meta data are created and attached to the original internal
 // symbols in the value profile annotation step
 // (PGOUseFunc::annotateIndirectCallSites). If a symbol does not have the meta
 // data, its original linkage must be non-internal.
 std::string getPGOFuncName(const Function &F, bool InLTO, uint64_t Version) {
   if (!InLTO) {
-    StringRef FileName = (StaticFuncFullModulePrefix
-                              ? F.getParent()->getName()
-                              : sys::path::filename(F.getParent()->getName()));
-    if (StaticFuncFullModulePrefix && StaticFuncStripDirNamePrefix != 0)
-      FileName = stripDirPrefix(FileName, StaticFuncStripDirNamePrefix);
+    StringRef FileName(F.getParent()->getSourceFileName());
+    uint32_t StripLevel = StaticFuncFullModulePrefix ? 0 : (uint32_t)-1;
+    if (StripLevel < StaticFuncStripDirNamePrefix)
+      StripLevel = StaticFuncStripDirNamePrefix;
+    if (StripLevel)
+      FileName = stripDirPrefix(FileName, StripLevel);
     return getPGOFuncName(F.getName(), F.getLinkage(), FileName, Version);
   }
 
   // In LTO mode (when InLTO is true), first check if there is a meta data.
   if (MDNode *MD = getPGOFuncNameMetadata(F)) {
     StringRef S = cast<MDString>(MD->getOperand(0))->getString();
     return S.str();
   }