[PATCH] D119916: Add a machine function pass to convert binop(phi(constants), v) to phi(binop)

[Guozhi Wei via Phabricator via llvm-commits]

Carrot added inline comments.


================
Comment at: llvm/lib/CodeGen/DupConstPhiUsers.cpp:10
+// \file This pass finds out PHIs whose operands are constants, duplicates its
+// user to predecessors and folds in the constants.
+//
----------------
qcolombet wrote:
> You mentioned in the description of the PR that this pass does both code size and performance improvements.
> May be worth saying that here again.
> 
> I haven't read the rest of the patch yet, but it would be good to describe the cost model as well somewhere.
> E.g., what would happen with something like:
> ```
> a = ldimm cst1
> if ()
>   b = ldimm cst2
> c = phi(a, b)
> = add ..., c
> ```
> 
> What prevents us for having 2 adds on the path `a -> b -> c`, which I believe would be a perf regression.
The main cost model checking and description is in the for loop of function FindCandidateInfo.

This patch will not transform this case because critical edge (a -> c) is not allowed, it is also explained in FindCandidateInfo.


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

https://reviews.llvm.org/D119916