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

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

> If not all the incoming values of a PHI node evaluate to the same constant then the PHI node cannot be folded.

Got it. Then it makes sense. Sorry for misreading.



================
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,
----------------
Let's mention the semantics of DeadBlocks here.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:246-247
+Constant *InstCostVisitor::visitPHINode(PHINode &I) {
+  if (I.getNumIncomingValues() > 4)
+    return nullptr;
+
----------------
What's this insight for limitation? I don't feel the new algorithm is super time consuming.


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


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:122-123
 
+    if (!DeadBlocks.insert(BB).second)
+      continue;
+
----------------
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.


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


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


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:258-260
+    if (!C) {
+      if (Inserted)
+        PendingPHIs.push_back(&I);
----------------
labrinea wrote:
> ChuanqiXu wrote:
> > What's the semantics for `PendingPHIs` here? It looks like a set of PHIs which would be constant but we don't know its values. But if it is this case, why can we insert the non-const and visited PHIs to `PendingPHIs`? It looks odd.
> Pending PHIs are phi nodes we have visited once without successfully folding them to a constant value, so we want to repeat this once more after all the specialization arguments have been estimated by the InstCostVisitor. It is possible that a later argument makes a conditional branch unconditional, and consequently a basic block turns dead (if it has a unique predecessor). As a result one of the incoming values of the PHI can be disregarded for the evaluation of the PHI. I'll add a comment explaining this.
Got it. It makes sense now. 


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:264
+    if (!Const)
+      Const = C;
+    else if (C != Const)
----------------
labrinea wrote:
> ChuanqiXu wrote:
> > Also I am curious why can we treat the PHI as constant with one of its incoming value? That doesn't make sense to me.
> `Const` is initialized to nullptr. The first incoming value that is identified to be constant sets `Const` to that value. If the remaing incomign values have that same constant value then we can replace the PHI, otherwise we can't. The only exception is if the incoming value is coming from a basic block we have proved is dead, or if the incoming values is this PHI itself, in which cases we ingore (skip) that incoming value. Perhaps I need to add a comment with this explanation.
Oh, I got it. It makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154852