<div dir="ltr"><div>Sorry - I missed your mail before I committed another small cleanup step:<br><a href="http://reviews.llvm.org/rL258446" rel="noreferrer" target="_blank">http://reviews.llvm.org/rL258446</a><br><br></div>Hopefully, we don't have any more bugs like PR26211 now, but I agree that there's more to do here. <br><br>We do the signature check for any FP type and then do it again to decide if a double->float shrink is applicable. I think we could have the helper return the type instead of a bool and avoid that. And as I noted in the commit message, we could hoist that check to optimizeCall(), so it's not duplicated all over the place. Another possibility would be to hoist the shrinking transforms from all of the individual optimization functions to optimizeCall(). I think it makes it more confusing to intermingle those optimizations.<br><br><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 21, 2016 at 2:48 PM, Davide Italiano <span dir="ltr"><<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Thu, Jan 21, 2016 at 3:19 PM, Sanjay Patel via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: spatel<br>
> Date: Thu Jan 21 14:19:54 2016<br>
> New Revision: 258428<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=258428&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=258428&view=rev</a><br>
> Log:<br>
> [LibCallSimplifier] don't get fooled by a fake fmin()<br>
><br>
> This is similar to the bug/fix:<br>
> <a href="https://llvm.org/bugs/show_bug.cgi?id=26211" rel="noreferrer" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=26211</a><br>
> <a href="http://reviews.llvm.org/rL258325" rel="noreferrer" target="_blank">http://reviews.llvm.org/rL258325</a><br>
><br>
> The fmin() test case reveals another bug caused by sloppy<br>
> code duplication. It will crash without this patch because<br>
> fp128 is a valid floating-point type, but we would think<br>
> that we had matched a function that used doubles.<br>
><br>
> The new helper function can be used to replace similar<br>
> checks that are used in several other places in this file.<br>
><br>
<br>
</span>This is nice. When I first started looking at LibCallSimplifier I<br>
noticed (and was disturbed) by all the duplicated checks, so I factored<br>
out some of them.<br>
Unfortunately there's still a lot more to do, so thanks for picking up<br>
the slack. I think at some point we might sit down and discuss how to<br>
refactor this properly.<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Davide<br>
<br>
"There are no solved problems; there are only problems that are more<br>
or less solved" -- Henri Poincare<br>
</font></span></blockquote></div><br></div>