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

[Stephen Lin]

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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fold-fops-into-selects.patch
Type: application/octet-stream
Size: 4671 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130719/dbe9b4b7/attachment.obj>