[PATCH] D56327: [PGO] Use SourceFileName rather module name in PGOFuncName

[Kristina Brooks via Phabricator via llvm-commits]

kristina requested changes to this revision.
kristina added a comment.
This revision now requires changes to proceed.

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;`.



================
Comment at: lib/ProfileData/InstrProf.cpp:255
   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 : -1;
----------------
davidxl wrote:
> xur wrote:
> > davidxl wrote:
> > > xur wrote:
> > > > davidxl wrote:
> > > > > When StaticFullModulePrefix is false, should the base name be used ?
> > > > 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.
> > > but the stripLevel will be reset to StaticFuncStripDirNamePrefix which is 0 by default?
> > It will not be reset as the cond in line 257 will be false if StripLevel is (unsigned) -1.
> ok, please put an explicit type casting before '-1' to make it clearer.
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).


================
Comment at: lib/ProfileData/InstrProf.cpp:256
+    StringRef FileName(F.getParent()->getSourceFileName());
+    uint32_t StripLevel = StaticFuncFullModulePrefix ? 0 : -1;
+    if (StripLevel < StaticFuncStripDirNamePrefix)
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56327/new/

https://reviews.llvm.org/D56327