[PATCH] D154852: [FuncSpec] Add Phi nodes to the InstCostVisitor.

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu accepted this revision.
ChuanqiXu added a comment.
This revision is now accepted and ready to land.

Thanks!



================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:760-762
       for (ArgInfo &A : S.Args)
         Score += getSpecializationBonus(A.Formal, A.Actual, Visitor);
+      Score += Visitor.getBonusFromPendingPHIs();
----------------
labrinea wrote:
> ChuanqiXu wrote:
> > labrinea wrote:
> > > ChuanqiXu wrote:
> > > > Given `getSpecializationBonus` would do additional computations exposing inlining opportunities exposing inlining via indirect call promotion., (e.g., converting the call to a function ptr to a direct function call), it looks like we didn't compute these for pending PHIs.
> > > > 
> > > > While the current patch is self-contained, maybe we can add a TODO/FIXME here for future improvements.
> > > As is, `getSpecializationBonus` only checks the specialization arguments (the root of the use-def chain) for inlining opportunities, not every single user of the use-def chain which might become constant. In that regard the pending PHIs are no different from the rest of the users that the InstCostVisitor is visiting through `getUserBonus`. Is this something we would like to change?
> > I think at least it is a potential optimization opportunities. Then if we have a consensus on this, it looks not bad to leave a TODO/FIXME.
> > 
> > > In that regard the pending PHIs are no different from the rest of the users that the InstCostVisitor is visiting through getUserBonus.
> > 
> > A difference I thought is that the PHIs are more possible to be a function pointers. (But now I don't think it is better to say "more possible" while both GEP and load instructions may get a constant too )
> In my opinion, I think it's better to add the FIXME under a separate commit, since it's not directly related to this patch as I've explained above.
That's fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154852