[PATCH] [RFC] Call FoldOpIntoSelect for all floating binops, not just fmul

[Nick Lewycky]

On 19 July 2013 08:41, Stephen Lin <swlin at post.harvard.edu> wrote:

> Hi,
>
> I ran an X86-64 test suite run and the only changes seemed to be
> normally distributed noise around 0.0% change.
>
> I believe this canonicalization is the right thing to do
> (unconditionally in the two-constant select case, and for consistency
> with existing practice in the one-constant select case). I also do not
> believe there is much of a possible impact in theory, given that these
> patterns are probably rare and, if present, should occur most often
> for multiplications (which are already canonicalized in this way)
> rather than additions, subtractions, or divisions.
>
> Does anyone have any objections, and can someone review for correctness?
>
> I will watch for regressions after this is committed, and if any pop
> up they can be addressed in the backends.
>

This patch looks great, please commit.

Nick


>
> Thanks,
> Stephen
>
> On Thu, Jul 18, 2013 at 1:43 PM, Stephen Lin <swlin at post.harvard.edu>
> wrote:
> > Hi,
> >
> > The function InstCombiner::FoldOpIntoSelect exists to fold binary
> > operations (and casts) with a select as an operand where at least one
> > operand of the select is a constant and the other operand of the
> > binary operation (if applicable) is a constant.
> >
> > Among floating math binops, it is only called for fmul, not fadd,
> > fsub, or fdiv. This seems at least partially an oversight rather than
> > something intentional, since the transformation is always profitable
> > if both operands of the select are constant as it eliminates the binop
> > completely.
> >
> > However, when only one operand of the select is a constant, the
> > transformation converts a binop with one operand as a select to a
> > select with one operand as a binop, and it's not as obvious which form
> > is better. The latter may expose other optimization opportunities if
> > the select can be further folded; however, if those opportunities do
> > not exist, it is possible that some hardware will perform the latter
> > more slowly than the former with current backend implementations.
> >
> > In any case, it seems like InstCombine ought to pick one over the
> > other for canonicalization and let the backends reverse the decision
> > if desired, and from the way InstCombiner::FoldOpIntoSelect is written
> > and called for other binops, it seems like the decision has already
> > been made that the latter should be preferred. So I am inclined to
> > just add calls to InstCombiner::FoldOpIntoSelect, as is, for all
> > floating binops; the patch attached does this.
> >
> > Anyway, I plan on running regression tests to see if there's any
> > adverse impact for this change, but if anyone has any insight or hints
> > on specific constructs/backends that should be investigated for
> > specific reasons, please let me know.
> >
> > Thanks,
> > Stephen
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130719/3dbbfb4d/attachment.html>