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

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

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.



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


================
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;
 
----------------
This smells not good. It looks dangerous that the `LastVisited` may be invalid.


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


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154852