[NFC] In InstrProf, generalize helper functions to take 'GlobalObject'. They currently take 'Functions' as function parameters or have 'Func' in the name. #70287

minglotus-6 · 2023-10-26T04:11:30Z

For instance, collectPGOFuncNameStrings is reused a lot in pr/66825 to collect vtable names; and in some added callsites it's just weird to see 'func' since context clearly shows it's not. This function currently just takes a list of strings as input so name it to collectGlobalObjectNameStrings
Do the rename in a standalone patch since the method is used in non-llvm codebase. It's easier to rollback this NFC in case rename in that codebase takes longer.

…ings - Do the rename in a standalone patch since the method is used in non-llvm codebase. It's easier to rollback this NFC in case rename in that codebase takes longer.

llvmbot · 2023-10-26T04:12:42Z

@llvm/pr-subscribers-pgo

Author: Mingming Liu (minglotus-6)

Changes

collectPGOFuncNameStrings is reused a lot in pr/66825 to collect vtable names. This function currently just takes a list of strings as input so name it to collectGlobalVariableNameStrings
Do the rename in a standalone patch since the method is used in non-llvm codebase. It's easier to rollback this NFC in case rename in that codebase takes longer.

Full diff: https://github.com/llvm/llvm-project/pull/70287.diff

3 Files Affected:

(modified) llvm/include/llvm/ProfileData/InstrProf.h (+4-4)
(modified) llvm/lib/ProfileData/InstrProf.cpp (+1-1)
(modified) llvm/unittests/ProfileData/InstrProfTest.cpp (+2-2)

diff --git a/llvm/include/llvm/ProfileData/InstrProf.h b/llvm/include/llvm/ProfileData/InstrProf.h
index f9096b46157200b..7832b4fbf35cc71 100644
--- a/llvm/include/llvm/ProfileData/InstrProf.h
+++ b/llvm/include/llvm/ProfileData/InstrProf.h
@@ -220,8 +220,8 @@ StringRef getPGOFuncNameVarInitializer(GlobalVariable *NameVar);
 StringRef getFuncNameWithoutPrefix(StringRef PGOFuncName,
                                    StringRef FileName = "<unknown>");
 
-/// Given a vector of strings (function PGO names) \c NameStrs, the
-/// method generates a combined string \c Result that is ready to be
+/// Given a vector of strings (names of global variables) \c NameStrs,
+/// the method generates a combined string \c Result that is ready to be
 /// serialized.  The \c Result string is comprised of three fields:
 /// The first field is the length of the uncompressed strings, and the
 /// the second field is the length of the zlib-compressed string.
@@ -229,8 +229,8 @@ StringRef getFuncNameWithoutPrefix(StringRef PGOFuncName,
 ///  third field is the uncompressed strings; otherwise it is the
 /// compressed string. When the string compression is off, the
 /// second field will have value zero.
-Error collectPGOFuncNameStrings(ArrayRef<std::string> NameStrs,
-                                bool doCompression, std::string &Result);
+Error collectGlobalVariableNameStrings(ArrayRef<std::string> NameStrs,
+                                       bool doCompression, std::string &Result);
 
 /// Produce \c Result string with the same format described above. The input
 /// is vector of PGO function name variables that are referenced.
diff --git a/llvm/lib/ProfileData/InstrProf.cpp b/llvm/lib/ProfileData/InstrProf.cpp
index e82cb5c535f1f5a..963d4703395b35b 100644
--- a/llvm/lib/ProfileData/InstrProf.cpp
+++ b/llvm/lib/ProfileData/InstrProf.cpp
@@ -545,7 +545,7 @@ Error collectPGOFuncNameStrings(ArrayRef<GlobalVariable *> NameVars,
   for (auto *NameVar : NameVars) {
     NameStrs.push_back(std::string(getPGOFuncNameVarInitializer(NameVar)));
   }
-  return collectPGOFuncNameStrings(
+  return collectGlobalVariableNameStrings(
       NameStrs, compression::zlib::isAvailable() && doCompression, Result);
 }
 
diff --git a/llvm/unittests/ProfileData/InstrProfTest.cpp b/llvm/unittests/ProfileData/InstrProfTest.cpp
index 76f88c058424d03..378795bd724b588 100644
--- a/llvm/unittests/ProfileData/InstrProfTest.cpp
+++ b/llvm/unittests/ProfileData/InstrProfTest.cpp
@@ -1368,7 +1368,7 @@ TEST_P(MaybeSparseInstrProfTest, instr_prof_symtab_compression_test) {
   for (bool DoCompression : {false, true}) {
     // Compressing:
     std::string FuncNameStrings1;
-    EXPECT_THAT_ERROR(collectPGOFuncNameStrings(
+    EXPECT_THAT_ERROR(collectGlobalVariableNameStrings(
                           FuncNames1,
                           (DoCompression && compression::zlib::isAvailable()),
                           FuncNameStrings1),
@@ -1376,7 +1376,7 @@ TEST_P(MaybeSparseInstrProfTest, instr_prof_symtab_compression_test) {
 
     // Compressing:
     std::string FuncNameStrings2;
-    EXPECT_THAT_ERROR(collectPGOFuncNameStrings(
+    EXPECT_THAT_ERROR(collectGlobalVariableNameStrings(
                           FuncNames2,
                           (DoCompression && compression::zlib::isAvailable()),
                           FuncNameStrings2),

david-xl

Since both Function and GlobalVariable are subclasses of GlobalObject, perhaps name it collectGlobalObjectNameStrings?

…cpp) that calls this function.

minglotus-6 · 2023-10-26T04:41:58Z

Since both Function and GlobalVariable are subclasses of GlobalObject, perhaps name it collectGlobalObjectNameStrings?

done.

david-xl · 2023-10-26T04:42:53Z

llvm/include/llvm/ProfileData/InstrProf.h

@@ -220,17 +220,17 @@ StringRef getPGOFuncNameVarInitializer(GlobalVariable *NameVar);
 StringRef getFuncNameWithoutPrefix(StringRef PGOFuncName,
                                   StringRef FileName = "<unknown>");

-/// Given a vector of strings (function PGO names) \c NameStrs, the
-/// method generates a combined string \c Result that is ready to be
+/// Given a vector of strings (names of global variables) \c NameStrs,


variable --> object

variable --> object

done.

teresajohnson · 2023-10-26T13:49:59Z

llvm/include/llvm/ProfileData/InstrProf.h

 ///  third field is the uncompressed strings; otherwise it is the
 /// compressed string. When the string compression is off, the
 /// second field will have value zero.
-Error collectPGOFuncNameStrings(ArrayRef<std::string> NameStrs,
-                                bool doCompression, std::string &Result);
+Error collectGlobalObjectNameStrings(ArrayRef<std::string> NameStrs,


There are a bunch of other PGOFuncName functions in this header file. Is this the only one that will be used for vtable profiling?

good question. I'll need to record the PGO name of vtables (i.e., just variable name if the vtable is external, and variable name with file path prefix if the vtable is local) jus like we record PGO name of functions (e.g., guid computed from global identifier is used for LTO) so a few other methods (with Func in function name, or Function as function paremeter type) needs to be generalized.

In this patch, added two small rename for static functions. The other collectPGOFuncNameStrings currently takes the global variable that consisting of a string literal (@__profn__Z4funcP4BaseP14FuncPtrWrapper = private constant [30 x i8] c"_Z4funcP4BaseP14FuncPtrWrapper" in the per-pass output of pgo-instr-gen in https://gcc.godbolt.org/z/97b6rzvvx) so I'll keep them there.

If more renames are needed later, I'm thinking of keeping the name getIRPGOFuncName since it's clearer for callers to know the callee is what they want (which reduces brain teasers to correlate 'Func' with 'Object' at least for me), and let it forward to an internal static function getIRPGONameForObject).

…gument type) to use them for general global objects (functions, vtable objects) 2. In InstrProf.h, add a function comment around the element in NameVars.

… to it

minglotus-6 · 2023-10-26T20:53:55Z

Per offline chat,

Added a static function getIRPGOObjectName and forwards getIRPGOFuncName to it. This way existing callers of getIRPGOFuncName remains unchanged, and the semantic (func vs object) is clearer at callsite.
Updated the comment of getPGOFuncName.

teresajohnson

lgtm with comment fix

teresajohnson · 2023-10-26T21:00:51Z

llvm/lib/ProfileData/InstrProf.cpp

+}
+
+// This is similar to `getIRPGOFuncName` except that this function calls
+// 'getIRPGOFuncName' to get a name and `getIRPGOFuncName` calls 'getIRPGOName'.


I think this should say "this function calls 'getPGOFuncName' to get a name and getIRPGOFuncName calls 'getIRPGONameForGlobalObject'? (note changes to both mentioned callees)

thanks for the catch. Updated it.

Local branch amd-gfx e08414e Merged main:0cbaff815cf2 into amd-gfx:ec3fdb3aaad4 Remote branch main a1e9777 [NFC] In InstrProf, generalize helper functions to take GlobalObject. They currently take Functions as function parameters or have Func in the name. (llvm#70287)

[NFC]Rename collectPGOFuncNameStrings to collectGlobalVariableNameStr…

04ead27

…ings - Do the rename in a standalone patch since the method is used in non-llvm codebase. It's easier to rollback this NFC in case rename in that codebase takes longer.

minglotus-6 requested a review from david-xl October 26, 2023 04:11

llvmbot added the PGO Profile Guided Optimizations label Oct 26, 2023

david-xl reviewed Oct 26, 2023

View reviewed changes

Address comments and update another callsite (in InstrProfCorrelator.…

0017907

…cpp) that calls this function.

minglotus-6 changed the title ~~[NFC]Rename collectPGOFuncNameStrings to collectGlobalVariableNameStrings~~ [NFC]Rename collectPGOFuncNameStrings to collectGlobalObjectNameStrings Oct 26, 2023

david-xl reviewed Oct 26, 2023

View reviewed changes

minglotus-6 added 2 commits October 25, 2023 22:20

update function comment

569b70a

run clang-format

be264c1

teresajohnson reviewed Oct 26, 2023

View reviewed changes

1. In InstrProf.cpp, change two static functions (function name or ar…

9a96d61

…gument type) to use them for general global objects (functions, vtable objects) 2. In InstrProf.h, add a function comment around the element in NameVars.

minglotus-6 changed the title ~~[NFC]Rename collectPGOFuncNameStrings to collectGlobalObjectNameStrings~~ [NFC] In InstrProf, generalize helper functions to take 'GlobalObject'. They currently take 'Functions' as function parameters or have 'Func' in the name. Oct 26, 2023

david-xl approved these changes Oct 26, 2023

View reviewed changes

Add static function 'getIRPGOObjectName'; forwards 'getIRPGOFuncName'…

a8c9965

… to it

teresajohnson approved these changes Oct 26, 2023

View reviewed changes

update comment per review comment

b776901

minglotus-6 merged commit a1e9777 into llvm:main Oct 26, 2023
1 of 3 checks passed

minglotus-6 mentioned this pull request Jan 4, 2024

[InstrProf] No linkage prefixes in IRPGO names #76994

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NFC] In InstrProf, generalize helper functions to take 'GlobalObject'. They currently take 'Functions' as function parameters or have 'Func' in the name. #70287

[NFC] In InstrProf, generalize helper functions to take 'GlobalObject'. They currently take 'Functions' as function parameters or have 'Func' in the name. #70287

minglotus-6 commented Oct 26, 2023 •

edited

Loading

llvmbot commented Oct 26, 2023

david-xl left a comment

minglotus-6 commented Oct 26, 2023

david-xl Oct 26, 2023

minglotus-6 Oct 26, 2023

teresajohnson Oct 26, 2023

minglotus-6 Oct 26, 2023

minglotus-6 commented Oct 26, 2023

teresajohnson left a comment

teresajohnson Oct 26, 2023

minglotus-6 Oct 26, 2023

[NFC] In InstrProf, generalize helper functions to take 'GlobalObject'. They currently take 'Functions' as function parameters or have 'Func' in the name. #70287

[NFC] In InstrProf, generalize helper functions to take 'GlobalObject'. They currently take 'Functions' as function parameters or have 'Func' in the name. #70287

Conversation

minglotus-6 commented Oct 26, 2023 • edited Loading

llvmbot commented Oct 26, 2023

david-xl left a comment

Choose a reason for hiding this comment

minglotus-6 commented Oct 26, 2023

david-xl Oct 26, 2023

Choose a reason for hiding this comment

minglotus-6 Oct 26, 2023

Choose a reason for hiding this comment

teresajohnson Oct 26, 2023

Choose a reason for hiding this comment

minglotus-6 Oct 26, 2023

Choose a reason for hiding this comment

minglotus-6 commented Oct 26, 2023

teresajohnson left a comment

Choose a reason for hiding this comment

teresajohnson Oct 26, 2023

Choose a reason for hiding this comment

minglotus-6 Oct 26, 2023

Choose a reason for hiding this comment

minglotus-6 commented Oct 26, 2023 •

edited

Loading