[PATCH] D94001: [CSSPGO] Call site prioritized inlining for sample PGO

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

Thanks for the data. It shows priority based inliner does significantly better than current early inliner in sample loader for CSSPGO both on performance and code size! It will interested to know how the importance to performance is distributed between priority based early inliner and CGSCC inliner now. I imagaine CGSCC inliner now plays a very minor role in CSSPGO now?



================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1412
+SampleProfileLoader::shouldInlineCandidate(InlineCandidate &Candidate) {
+  assert(ProfileIsCS);
+
----------------
Add assert message. There are some other places missing the messages.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1535
+        // For indirect call, we don't run CallAnalyzer through InlineCost
+        // before actual inlining to work around PR18962. However, that means we
+        // may do ICP first and later decided not to inline, which is mostly ok
----------------
According to https://bugs.llvm.org/show_bug.cgi?id=18962, PR18962 has been fixed  in 2014. Is it the right bug to refer to?


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1544-1546
+          // Attach function profile for selected indirect callee, and update
+          // call site count for the selected target too. Speculatively check
+          // if it's beneficial to inline the callee to decide whether to ICP.
----------------
Should the comment block be hoisted?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94001