<div dir="ltr">On 19 July 2013 08:41, Stephen Lin <span dir="ltr"><<a href="mailto:swlin@post.harvard.edu" target="_blank">swlin@post.harvard.edu</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
I ran an X86-64 test suite run and the only changes seemed to be<br>
normally distributed noise around 0.0% change.<br>
<br>
I believe this canonicalization is the right thing to do<br>
(unconditionally in the two-constant select case, and for consistency<br>
with existing practice in the one-constant select case). I also do not<br>
believe there is much of a possible impact in theory, given that these<br>
patterns are probably rare and, if present, should occur most often<br>
for multiplications (which are already canonicalized in this way)<br>
rather than additions, subtractions, or divisions.<br>
<br>
Does anyone have any objections, and can someone review for correctness?<br>
<br>
I will watch for regressions after this is committed, and if any pop<br>
up they can be addressed in the backends.<br></blockquote><div><br></div><div>This patch looks great, please commit.</div><div><br></div><div>Nick</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


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