[PATCH] D108437: [CSSPGO] split context string III - llvm-profgen changes

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/test/tools/llvm-profgen/recursion-compression-pseudoprobe.test:12
+
+; CHECK-UNCOMPRESS: [main:2 @ foo:5 @ fa:8 @ fa:7 @ fb:5 @ fb:5 @ fb:5 @ fb:5 @ fb:5 @ fb:5 @ fb:5 @ fb:5 @ fb:6 @ fa]:4:1
 ; CHECK-UNCOMPRESS:  1: 1
----------------
wenlei wrote:
> This order change is due to sortFuncProfiles change in D108435? Does the test change belong to this MD5 patch or D108435? 
The order change is due to the flip of using `>` to `<` for `sortFuncProfiles` in D108433, so that we don't have to provide a `>` operator.


================
Comment at: llvm/tools/llvm-profgen/CallContext.h:21
 // Function name, LineLocation
-typedef std::pair<std::string, LineLocation> FrameLocation;
+using SampleContextFrameVector = SampleContextFrameVector;
 
----------------
wenlei wrote:
> Does this using statement do anything?
oops, looks like it's from a string replacement. Removed.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:423
+  } else {
+    llvm_unreachable("NYI");
   }
----------------
wenlei wrote:
> nit: NYI is confusing. This is simply not expected, instead of "Not yet implemented", right? we don't have a 3rd ContextKey type.
Makes sense. Message changed.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:374
+        getCallerContext(CalleeContext.getContextFrames(), CallerContextId);
+    SampleContextFrames CallerContextRef(CallerContextId);
 
----------------
wenlei wrote:
> The name CallerContextRef probably is left over from before we renamed into SampleContextFrames. Rename CallerContextRef accordingly, same for other places. And all other places where Context is called Name better be renamed accordingly too.
Good catch. Renamed to `CallerFrames`.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:374
+        getCallerContext(CalleeContext.getContextFrames(), CallerContextId);
+    SampleContextFrames CallerContextRef(CallerContextId);
 
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > The name CallerContextRef probably is left over from before we renamed into SampleContextFrames. Rename CallerContextRef accordingly, same for other places. And all other places where Context is called Name better be renamed accordingly too.
> > Good catch. Renamed to `CallerFrames`.
> getFunctionProfileForLeafProbe has two instance of ContextRef still.
Renamed to `NewContext`.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:425
+    SampleContextFrame Callsite(*It.first, Line);
     CallStack.push_back(Callsite);
   }
----------------
wenlei wrote:
> emplace_back with variadic args and avoid temp Callsite
fixed


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:284
+      InlineContextStack.push_back(
+          SampleContextFrame(Callsite.first, LineLocation(Callsite.second, 0)));
+    }
----------------
wenlei wrote:
> The actual string for Callsite.first is owned by MCPseudoProbeDecoder, right? Specifically, GUID2FuncDescMap.
Yes, the string is owned by the probe decoder, unlike the strings returned from the symbolizer which have to be saved somewhere in the profiled binary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108437