[PATCH] D154852: [FuncSpec] Add Phi nodes to the InstCostVisitor.
Chuanqi Xu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 10 19:22:59 PDT 2023
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
More information about the llvm-commits
mailing list