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

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea added a comment.

In D154852#4487493 <https://reviews.llvm.org/D154852#4487493>, @ChuanqiXu wrote:

> The motivation of the patch looks a little bit odd to me.
>
> Given:
>
>   a:
>     br label %merge
>   
>   b:
>     br label %merge
>   
>   merge:
>     %p = phi i32 [%arg0, %a], [%non_constant, %b]
>
> Given `%arg0` may be constant , I don't think the information implies (any) benefits with function specialization. Because after the transformation, `%p` won't be constant still.
>
> I know there is an optimization (may be aggressive JumpThreading?) to optimize the case:
>
>   a:
>     br label %merge
>   
>   b:
>     br label %merge
>   
>   merge:
>     %p = phi i32 [%constant, %a], [%non_constant, %b]
>     use of %p
>     goto successors
>
> to
>
>   a:
>     br label %merge.a
>   
>   b:
>     br label %merge
>   
>   merge:
>     %p = phi i32  [%non_constant, %b]
>     use of %p
>     goto successors
>   
>   merge.a:
>      %p.a = %constant
>      use of %constant
>      goto copy of successors
>
> But I am not sure if this is implemented in LLVM. Also it smells not good to make FS to dependent on such optimizations.

Apologies for the confusion, perhaps I need to add some comments explaining the intention. If not all the incoming values of a PHI node evaluate to the same constant then the PHI node cannot be folded.



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


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


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:255-257
+    auto *C = dyn_cast<Constant>(V);
+    if (!C)
+      C = findConstantFor(V, KnownConstants);
----------------
ChuanqiXu wrote:
> Maybe it is slightly better to alert the `findConstantFor `.
Ack


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:258-260
+    if (!C) {
+      if (Inserted)
+        PendingPHIs.push_back(&I);
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:264
+    if (!Const)
+      Const = C;
+    else if (C != Const)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154852