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

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea marked 8 inline comments as done.
labrinea added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:105
 // the combination of size and latency savings in comparison to the non
 // specialized version of the function.
 static Cost estimateBasicBlocks(SmallVectorImpl<BasicBlock *> &WorkList,
----------------
ChuanqiXu wrote:
> Let's mention the semantics of DeadBlocks here.
I'll improve the wording.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:246-247
+Constant *InstCostVisitor::visitPHINode(PHINode &I) {
+  if (I.getNumIncomingValues() > 4)
+    return nullptr;
+
----------------
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.


================
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();
----------------
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?


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:122-123
 
+    if (!DeadBlocks.insert(BB).second)
+      continue;
+
----------------
ChuanqiXu wrote:
> labrinea wrote:
> > ChuanqiXu wrote:
> > > What's the semantics of `DeadBlocks` here? Why would we add `BB` to DeadBlocks? It looks odd to me since it implies that estimating basic blocks will mark these blocks as dead.
> > This function is called when we constant fold switch instructions or conditional branches. It estimates the cost of instructions residing in dead blocks (unless they've already been estimated while constant folding).
> I got it. The semantics is "these blocks are dead if we convert the visiting values to constant. (But we haven't decided we're going to transform them, right?)". It is still slightly odd for me. But I don't have a suggestion here.
The `DeadBlocks` is a member of the `InstCostVisitor`, which by definition only estimates, does not transform itself. I'll make it clear on the description that here we are estimating the benefit to later decide whether we are going to make a transformation.


================
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;
 
----------------
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())`


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:253-254
+    if (auto *Inst = dyn_cast<Instruction>(V))
+      if (DeadBlocks.contains(Inst->getParent()) || Inst == &I)
+        continue;
+    auto *C = dyn_cast<Constant>(V);
----------------
ChuanqiXu wrote:
> labrinea wrote:
> > I am wondering whether we should be checking `DeadBlocks.contains(I.getIncomingBlock(Idx))`. Imagine this scenario:
> > 
> > 
> > ```
> >         x = 2
> >        /     \
> >       /    aliveBB
> >   deadBB
> >       \     y = 3
> >        \     /
> >   phi ( x , y )
> > ```
> > The basic block where x is defined is alive, therefore we can't fold the PHI because `x != y`. Is it not a pessimization?
> I don't understand this. Given DeadBlocks contains `deadBB` and `I.getIncomingBlock(Idx)` for `x` returns deadBB, we won't consider the value from `x`, right? Then it doesn't matter if `x != y`. Do I understand correctly?
Exactly. The previous revision was checking `if (DeadBlocks.contains(Inst->getParent())`, which was wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154852