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

[Wenlei He via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:876
   }
-  InlineFunctionInfo IFI(nullptr, &GetAC);
+  InlineFunctionInfo IFI(nullptr, &GetAC, nullptr, nullptr, nullptr, false);
   if (InlineFunction(CS, IFI)) {
----------------
tejohnson wrote:
> Document constant parameters.
Reverted this as part of the split.


================
Comment at: llvm/lib/Transforms/Utils/InlineFunction.cpp:1723
+    if (IFI.CallerBFI != nullptr && IFI.CalleeBFI != nullptr &&
+        IFI.UpdateProfile)
       // Update the BFI of blocks cloned into the caller.
----------------
tejohnson wrote:
> Can you split this into a separate patch (with a test) since it isn't related to the headline of this patch?  Are we currently getting incorrect profiles without this fix?
> 
> Suggest pulling the if (IFI.UpdateProfile) checks out into a single if for clarity.
Sure.. the profile we are getting is still incorrect for some cases because line 1517 is still executed for inlining with CS profile. But I will put that into a separate diff later with its own test case. 


================
Comment at: llvm/test/Transforms/SampleProfile/inline-callee-update.ll:1
+; facebook T56498894: Make sure Import GUID list for ThinLTO properly maintained while update function's entry count for inlining
+
----------------
tejohnson wrote:
> Is this intended to be here?
oops.. removed internal marker, but left the comment there.


================
Comment at: llvm/test/Transforms/SampleProfile/inline-callee-update.ll:6
+define i32* @sample_loader_inlinee() !dbg !6{
+  %1 = call i32* @direct_leaf_func(i32* null), !dbg !7
+  %cmp = icmp ne i32* %1, null
----------------
wmi wrote:
> Please use "opt -instnamer" to change %1, %2, ... to names more than just numbers.
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69736