[PATCH] D70655: [AutoFDO] Top-down Inlining for specialization with context-sensitive profile

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

That is a good catch. From my understanding it could potentially reduce inlining in some cold places and potentially increase regular inlinling. Do you see how much impact on performance and code size by changing it? I will do some evaluation on my side.



================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:429-430
 
+  // Order list that dictates annotation and inlining order from sample loader
+  std::vector<Function *> FunctionOrderList;
+
----------------
If the buildFunctionOrder call is moved to runOnModule, we don't need the field and buildFunctionOrder can return a function order vector.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1703
+    for (CallGraphNode *node : *CGI) {
+      if (node->getFunction())
+        FunctionOrderList.push_back(node->getFunction());
----------------
We can move the check about whether the Function is a declaration to here, so FunctionOrderList can be a little bit smaller.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1788
+  for (auto F : FunctionOrderList)
+    if (!F->isDeclaration()) {
       clearFunctionData();
----------------
The check can be moved above.


================
Comment at: llvm/test/Transforms/SampleProfile/inline-topdown.ll:18
+  store i32 %y, i32* %y.addr, align 4
+  %0 = load i32, i32* %x.addr, align 4, !dbg !11
+  %1 = load i32, i32* %y.addr, align 4, !dbg !11
----------------
rename the variable using opt -instnamer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70655