[PATCH] D39869: [Inliner] Inline through indirect call sites having !callees metadata

[Easwaran Raman via Phabricator via llvm-commits]

eraman added a comment.

This is looking good. Main comment I have is about the number of promotions+inlining that can take place is unbounded. One could argue that if there were many small direct calls, we would inline all of them, but still I feel it is better to limit the number of promotions.



================
Comment at: lib/Transforms/IPO/Inliner.cpp:563
+      // call site will now be the direct one the indirect one will be visited
+      // again.
+      Calls.push_back({DirectCS, Calls[Index].second});
----------------
Is there any limit on the size of the callees metadata? Otherwise you might potentially end up promoting a large number of calls and inline them


================
Comment at: lib/Transforms/IPO/Inliner.cpp:857
+      // demote it.
+      Guard.setInlined();
+
----------------
Nit: The call is not necessarily inlined. More precisely, the call is deleted (either due to inlining or because it is dead). Perhaps just modify the comment above or rename the function.


================
Comment at: lib/Transforms/Utils/CallPromotionUtils.cpp:621
   // be executed.
-  Instruction *NewInst = versionCallSite(CS, Callee, BranchWeights);
+  Instruction *NewInst = versionCallSite(CS, Callee, CPI, BranchWeights);
 
----------------
You could set the CPI->Versioned = true here instead of passing an additional parameter to versionCallSite.


================
Comment at: lib/Transforms/Utils/CallPromotionUtils.cpp:650
+    Function *F = PossibleCallees[0];
+    if (CalleesToIgnore.count(F) || !isLegalToPromote(CS, F))
+      return nullptr;
----------------
It looks like isLegalToPromote doesn't check if the callee's body is available. If F->isDeclaration() is true, is there any reason to promote? The inliner is going to not inline the callee and ends up unnecessarily promote and demote the callee. This is even more important in the versioned case since it invalidates the analysis.


================
Comment at: test/Transforms/Inline/callees-metadata.ll:22
+; CHECK:         [[TMP0:%.*]] = icmp eq i64 (i64, i64)* %fp, @select_def_0
+; CHECK-NEXT:    [[TMP1:%.*]] = select i1 [[TMP0]], i64 %x, i64 %y
+; CHECK-NEXT:    ret i64 [[TMP1]]
----------------
This is relying on how instructions get simplified after inlining. Why not check that there are not call instructions and followed by a ret?


Repository:
  rL LLVM

https://reviews.llvm.org/D39869