[PATCH] D69736: Keep import function list for inlinee profile update

[Wenlei He via Phabricator via llvm-commits]

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

> I agree setEntryCount always copying the existing import list is better than adding updateEntryCount. Adding a separate metadata also sounds good to me. I don't know the history why enbedding import list in function_entry_count. @tejohnson may know.

Updated the diff to have `setEntryCount` always copying the existing import list for now. I'd be happy to add a new metadata for import list too.. Thanks for reviewing!



================
Comment at: llvm/include/llvm/Transforms/Utils/Cloning.h:208-211
+  /// Update profile for callee as well as cloned version. We need to do this
+  /// for regular inlining, but not for inlining from sample profile loader.
+  bool UpdateProfile;
+
----------------
wmi wrote:
> I don't see where UpdateProfile is used. 
oops, merge error, thanks for catching this. fixed. 


================
Comment at: llvm/test/Transforms/SampleProfile/inline-callee-update.ll:77-80
+; CHECK: distinct !DISubprogram(name: "sample_loader_inlinee"
+; CHECK-NEXT: {!"function_entry_count", i64 1, i64 -1383577875507729398}
+; CHECK: distinct !DISubprogram(name: "cgscc_inlinee"
+; CHECK-NEXT: !{!"function_entry_count", i64 0, i64 -1383577875507729398}
----------------
wmi wrote:
> I am curious why these two functions have non empty import lists because all the functions are defined within the module. 
> 
> Then I find indirect call targets with inline instance are all added to import list. That is unnecessary if the call targets are defined in current module. That could be improved. At least, the test shouldn't depend on it. 
> 
> 
Makes sense, changed the test case to rely only on cross module (declare-only) import. (we can trim same module indirect callee import in a separate diff)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69736