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

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

Thanks for the patch!

> This is a minimal point fix (was trying to keep internal patch minimum), but it might actually make sense to have `setEntryCount` always copy over the existing import list (if it exists), instead of adding `updateEntryCount`? Or even further, we could have a separate metadata for the import list, instead of piggyback on `function_entry_count`? To me, it seems a bit weird to embed import list as part of `function_entry_count` - wondering what's the reason behind that?

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.



================
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;
+
----------------
I don't see where UpdateProfile is used. 


================
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}
----------------
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. 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69736