[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:68-69
       Queue.pop();
-      FunctionSamples *CallerSamples = Caller->getFunctionSamples();
-
-      // Add calls for context, if both caller and callee has context profile.
+      // Add calls for context. When AddNodeWithSamplesOnly is true, both caller
+      // and callee need to have context profile.
+      // Callsite target samples are not considered. A call target that doesn't
----------------
Remove AddNodeWithSamplesOnly parameter and always add everything even for llvm-profgen? CSPreInliner::processFunction skips names without profile, so it should just work. llvm-profgen should follow what compiler does.


================
Comment at: llvm/include/llvm/Transforms/IPO/ProfiledCallGraph.h:70-73
+      // Callsite target samples are not considered. A call target that doesn't
+      // come with a profile or is not on a call path to its child profile can
+      // be ignored, since processing it before its caller (if this is the only
+      // context) should not cause a problem.
----------------
Actually adding extra edges from call target samples has the risk of forming top-down order that is not compliant with context order due to SCC, right? That is similar to how adding static edges can hurt.

So instead of saying call target edge doesn't help, it would be helpful if we make it clear in the comment explaining how adding extra edges from call targets may hurt.


================
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:
> hoy wrote:
> > 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?
> Silently ignoring this flag may cause confusion. A warning may be enough. 
Looks like this is not enforced though.. A common pattern is tuning knobs for an optimization and when optimization is turned off, we don't emit warning when tuning flags are still used. It looks to me that silently ignore a tuning flag when an optimization is off is more mainstream then emitting a warning.. don't have a strong opinion though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99351