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

[Wenlei He via Phabricator via llvm-commits]

wenlei marked 3 inline comments as done.
wenlei added inline comments.


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


================
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
----------------
wmi wrote:
> 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?
The bug was fixed, but we generate different types for the same definition for the case in that bug, and call analyzer can't handle that now. Updated the comment to include more details.

For code below, whether fn1 is present affects the type name of A. Then we could see two different types from the same definition, which makes CallAnalyzer choke as it's expecting matching parameter type on both caller and callee side.

```
class A {
  // append has to have the same prototype as fn1 to tickle the bug.
  void (*append)(A *);
};
void fn1(A *p1) {
}
```



================
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.
----------------
wmi wrote:
> Should the comment block be hoisted?
Updated the 1st part of the comment, and removed the 2nd part which is out dated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94001