[PATCH] D99351: [CSSPGO] Top-down processing order based on full profile.

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h:46
+  // Constructor for non-CS profile.
+  ProfiledCallGraph(SampleProfileReader &Reader) {
+    assert(!Reader.profileIsCS() && "CS profile is not handled here");
----------------
Can we just pass in `StringMap<FunctionSamples> &ProfileMap` instead of the `Reader`?  ProfiledCallGraph doesn't need to interact with Reader except for getting the profiles. 

For checking CS profile, we could do FunctionsSamples::ProfileIsCS too.


================
Comment at: llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h:54
+  // Constructor for CS profile.
+  ProfiledCallGraph(SampleContextTracker &ContextTracker) {
+    // BFS traverse the context profile trie to add call edges for calls shown
----------------
Can we merge the two ctors for CS profile? Some refactoring to merge them, with parameter to control whether we add edges for call targets and comments explaining why call target edges are skipped would be good. 

It also makes sense for llvm-profgen path to use exactly what the compiler uses for top-down ordering. So if adding call target edges are problematic for compiler, maybe we should skip that for llvm-profgen too.


================
Comment at: llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h:76
     // We only add function with actual context profile
     for (auto &FuncSample : ProfileMap) {
       FunctionSamples *FSamples = &FuncSample.second;
----------------
The purpose of this is to have all nodes populated before adding call edges and enable sanity check when adding edges. But I can see that's not possible for afdo profiles. If we end up adding nodes on the fly for afdo, we could do the same for csspgo, in which case we can remove ProfileMap from parameter. 


================
Comment at: llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h:131
 
+  void addProfiledCallEdges(const FunctionSamples &Samples) {
+    addProfiledFunction(Samples.getFuncName());
----------------
If we name the one above as `addProfiledCall`, this would be `addProfiledCalls` to be consistent? 

(And this is indeed adding both nodes and edges)


================
Comment at: llvm/include/llvm/Transforms/IPO/SampleContextTracker.h:21
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Analysis/CallGraph.h"
 #include "llvm/IR/DebugInfoMetadata.h"
----------------
This can be removed too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99351