[PATCH] D156569: [InstrProf] Encode linkage names in IRPGO counter names

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

Looks reasonable to me. Thanks for fixing this FIXME

  // GlobalValue::getGlobalIdentifier() prefixes the filename if the symbol
  // is local. This logic will break if there is a colon in the filename,
  // but we cannot use rsplit() because ObjC symbols can have colons.



================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:313
+// See getPGOFuncName()
+std::string getIRPGOFuncName(const Function &F, bool InLTO) {
+  if (!InLTO) {
----------------
https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:358
 
+// See getIRPGOFuncName() for a discription of the format
+std::pair<StringRef, StringRef>
----------------



================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:360
+std::pair<StringRef, StringRef>
+getParsedIRPGOFuncName(StringRef IRPGOFuncName) {
+  if (IRPGOFuncName.contains(';'))
----------------



================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:361
+getParsedIRPGOFuncName(StringRef IRPGOFuncName) {
+  if (IRPGOFuncName.contains(';'))
+    return IRPGOFuncName.split(';');
----------------
`contains` and `split` duplicate the work to search `;`. You may just call `split` and check whether the second element is empty.


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:386
   // Now fix up illegal chars in local VarName that may upset the assembler.
-  const char *InvalidChars = "-:<>/\"'";
+  const char *InvalidChars = "-:;<>/\"'";
   size_t found = VarName.find_first_of(InvalidChars);
----------------
while you are here


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:1224
+    // If we don't find FuncName, try DeprecatedFuncName to handle profiles
+    // built by older compilers
+    auto Err2 =
----------------



================
Comment at: llvm/test/Transforms/PGOProfile/statics_counter_naming.ll:8
+; NOPATH: @__profn_statics_counter_naming.ll_func = private constant [30 x i8] c"statics_counter_naming.ll;func"
+; HASPATH-NOT: @__profn_statics_counter_naming.ll_func = private constant [30 x i8] c"statics_counter_naming.ll;func"
 
----------------
This line tests `-static-func-strip-dirname-prefix=1` by asserting that the NOPATH line does not occur. This is a bit weak. 
It's probably better to `rm -rf %t && mkdir -p %t/dir && cd %t; cp %s dir/a.ll; opt ... dir/a.ll` so that we can actually check `-static-func-strip-dirname-prefix=1 `, but such a change is not the scope of this patch.


================
Comment at: llvm/unittests/ProfileData/InstrProfTest.cpp:526
+  auto M = std::make_unique<Module>("MyModule.cpp", Ctx);
+  DataLayout DL("m:o"); // macho
+  M->setDataLayout(DL);
----------------
```
// Use Mach-O mangling that non-private symbols get a `_` prefix. 
M->setDataLayout(DataLayout("..."))
```



================
Comment at: llvm/unittests/ProfileData/InstrProfTest.cpp:537
+  Data.emplace_back("WeakODRFoo", Function::WeakODRLinkage, "_WeakODRFoo");
+  Data.emplace_back("\01-[C dynamicFoo:]", Function::ExternalLinkage,
+                    "-[C dynamicFoo:]");
----------------
Add a comment that they are ObjC symbols.


================
Comment at: llvm/unittests/ProfileData/InstrProfTest.cpp:549
+    auto *F = M->getFunction(Name);
+    ASSERT_NE(F, nullptr);
+    auto IRPGOFuncName = getIRPGOFuncName(*F);
----------------
Personally I think this ASSERT_NE can be removed as `*F` expresses the intention.


================
Comment at: llvm/unittests/ProfileData/InstrProfTest.cpp:595
+  auto M = std::make_unique<Module>("MyModule.cpp", Ctx);
+  DataLayout DL("m:o"); // macho
+  M->setDataLayout(DL);
----------------
```
// Use Mach-O mangling that non-private symbols get a `_` prefix. 
M->setDataLayout(DataLayout("..."))
```


================
Comment at: llvm/unittests/ProfileData/InstrProfTest.cpp:599
+  auto *FooF = Function::Create(FTy, Function::InternalLinkage, "Foo", M.get());
+  auto *BarF = Function::Create(FTy, Function::InternalLinkage, "Bar", M.get());
+
----------------
Perhaps a non-internal linkage for more variance.


================
Comment at: llvm/unittests/ProfileData/InstrProfTest.cpp:1322
+              getParsedIRPGOFuncName(IRPGOFuncName).second);
+    // Ensure we can still read this old record name
     std::string PGOName = getPGOFuncName(*F);
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156569