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

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:246-247
+Constant *InstCostVisitor::visitPHINode(PHINode &I) {
+  if (I.getNumIncomingValues() > 4)
+    return nullptr;
+
----------------
labrinea wrote:
> ChuanqiXu wrote:
> > What's this insight for limitation? I don't feel the new algorithm is super time consuming.
> There can be phi nodes with 64 incoming values or more, in which case it can be time consuming. We do need to early exit. I think 4 is a reasonable number.
While I agree with you, it is better to make this an option. Then we reduce the magic number and have better semantics here even if we can image that there won't be so many people tuning on that.


================
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:
> > 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 )


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:168-169
   // Cache the iterator before visiting.
-  LastVisited = KnownConstants.insert({Use, C}).first;
+  if (Use)
+    LastVisited = KnownConstants.insert({Use, C}).first;
 
----------------
labrinea wrote:
> ChuanqiXu wrote:
> > labrinea wrote:
> > > ChuanqiXu wrote:
> > > > This smells not good. It looks dangerous that the `LastVisited` may be invalid.
> > > The function `getBonusFromPendingPHIs` just above contains the only callsite to `getUserBonus` where nullptr is passed for the last two arguments. The key to a DenseMap cannot be nullptr. This isn't problematic as `visitPHINode` doesn't use `LastVisited`.
> > I know it. I just feel the pattern is dangerous. For example, in all other visit* methods, we always assume the `LastVisited` is valid, but it won't be true after this patch. (Although it is not formally stated). The concern I had was that we broke the assumption here. And it is almost bad to broke some assumptions. I think the easy solution here may insert:
> > 
> > ```
> > if (LastVisited == KnownConstants.end())
> >     return nullptr;
> > ```
> > 
> > to many other visit* methods. And set `LastVisited` to `KnownConstants.end()` if the `use` and `C` is nullptr.
> There are other visit methods which are already not using the `LastVisited` iterator like `visitCallBase` and `visitGetElementPtrInst`. I think the extra checks are expensive, perhaps we could do:
> 
> ```
> LastVisited = Use ? KnownConstants.insert({Use, C}).first : KnownConstants.end();
> ```
> then inside the visit methods `assert(LastVisited != KnownConstants.end())`
Yeah, that sounds much better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154852