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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet 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.
+//
----------------
Carrot wrote:
> 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.
Okay I'll take a closer look, but here `a -> c` is not a critical edge.


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

https://reviews.llvm.org/D119916