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

[Hongtao Yu via Phabricator via llvm-commits]

hoy marked 2 inline comments as done.
hoy added a comment.

In D99351#2657349 <https://reviews.llvm.org/D99351#2657349>, @wmi wrote:

> The performance test result is neutral, so I think we can enable UseProfiledCallGraph by default.

Thanks for measuring the performance. Sounds good to turn it on by default.



================
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");
----------------
wenlei wrote:
> 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.
Good point, `Reader` is not really needed here.


================
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
----------------
wenlei wrote:
> 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.
Yes, they can be merged. The existing constructor will need to get rid of ProfileMap as you mentioned in the other comment.


================
Comment at: llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h:131
 
+  void addProfiledCallEdges(const FunctionSamples &Samples) {
+    addProfiledFunction(Samples.getFuncName());
----------------
wenlei wrote:
> If we name the one above as `addProfiledCall`, this would be `addProfiledCalls` to be consistent? 
> 
> (And this is indeed adding both nodes and edges)
Sounds good.


================
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"
----------------
wenlei wrote:
> This can be removed too.
Good catch.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:165
+static cl::opt<bool>
+    UseProfiledCallGraph("use-profiled-call-graph", cl::init(false), cl::Hidden,
+                         cl::desc("Process functions in a top-down order "
----------------
wmi wrote:
> Emit an error to prevent misuse if ProfileTopDownLoad is false and UseProfiledCallGraph is true?
Actually when ProfileTopDownLoad is false, UseProfiledCallGraph doesn't do anything since it'll return early in `buildProfiledCallGraph`. Do you think an error is needed when it returns early while UseProfiledCallGraph is true?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99351