[PATCH] D44626: [InstCombine] Fold (A OR B) AND B code sequence over Phi node

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

In https://reviews.llvm.org/D44626#1081987, @spatel wrote:

> In https://reviews.llvm.org/D44626#1080274, @efriedma wrote:
>
> > I'm still not sure this is really as general as it could be, but I guess it's okay.
>
>
> I think what this patch really wants to ask/do is: "Does this binop simplify with the incoming value of the phi to 1 of the binop operands? If yes, substitute that value into the phi."
>
> If you look at it that way, then it should be some add-on to the existing foldOpIntoPhi() - and that's probably a smaller and more general patch.
>
> That substitution analysis seems to fall into the gray area -- or not gray depending on your viewpoint :) -- of whether this belongs in (New)GVN or instcombine. (cc @fhahn).


FWIW, the underlying issue for me in doing it in instcombine is that it can never really be good at it in any sane time bound.  Without knowing the values of other instructions ahead of time (IE some form of value numbering),  it has to go figure them out (again and again), or give up and only tackle simple cases around constants (which people never stay happy with).  It also has no fine grain dependency tracking, so it will re-evaluate this all the time.  By contrast, NewGVN (or even GVN) only re-evaluate these transforms at the points they could change, and already know the values of other things in the program, so can tell when the transform produces a redundancy without further evaluation.

Over time, either the set of cases you catch suck, you make instcombine slow and complicated, or you make instcombine do value numbering.
None of these seem like appealing options to me ;)


https://reviews.llvm.org/D44626