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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/CallContext.h:21
 // Function name, LineLocation
-typedef std::pair<std::string, LineLocation> FrameLocation;
+using SampleContextFrameVector = SampleContextFrameVector;
 
----------------
Does this using statement do anything?


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


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:374
+        getCallerContext(CalleeContext.getContextFrames(), CallerContextId);
+    SampleContextFrames CallerContextRef(CallerContextId);
 
----------------
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.


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


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:283
+    for (auto &Callsite : ProbeInlineContext) {
+      InlineContextStack.push_back(
+          SampleContextFrame(Callsite.first, LineLocation(Callsite.second, 0)));
----------------
nit: emplace_back with variadic args 


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:284
+      InlineContextStack.push_back(
+          SampleContextFrame(Callsite.first, LineLocation(Callsite.second, 0)));
+    }
----------------
The actual string for Callsite.first is owned by MCPseudoProbeDecoder, right? Specifically, GUID2FuncDescMap.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108437