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

[Rong Xu via Phabricator via llvm-commits]

xur marked 2 inline comments as done.
xur added inline comments.


================
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;
----------------
kristina wrote:
> 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).
it's to set to the largest int. In the committed version, there is an explicit conversion.


================
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;
----------------
xur wrote:
> kristina wrote:
> > 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).
> it's to set to the largest int. In the committed version, there is an explicit conversion.
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.


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

https://reviews.llvm.org/D56327