I think the patch is safe, but this bug has probably existed for a long time. So it's a judgement call whether it should be merged to 3.8?<br><br>On Thursday, January 21, 2016, Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Sanjay, it was pointed out by Joerg that this should probably also be<br>
merged to 3.8?<br>
<br>
On Thu, Jan 21, 2016 at 12:19 PM, Sanjay Patel via llvm-commits<br>
<<a href="javascript:;" onclick="_e(event, 'cvml', '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" 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" target="_blank">https://llvm.org/bugs/show_bug.cgi?id=26211</a><br>
> <a href="http://reviews.llvm.org/rL258325" 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>
> Modified:<br>
> llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp<br>
> llvm/trunk/test/Transforms/InstCombine/double-float-shrink-1.ll<br>
><br>
> Modified: llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp?rev=258428&r1=258427&r2=258428&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp?rev=258428&r1=258427&r2=258428&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp (original)<br>
> +++ llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp Thu Jan 21 14:19:54 2016<br>
> @@ -953,13 +953,34 @@ static Value *valueHasFloatPrecision(Val<br>
> return nullptr;<br>
> }<br>
><br>
> +/// Any floating-point library function that we're trying to simplify will have<br>
> +/// a signature of the form: fptype foo(fptype param1, fptype param2, ...).<br>
> +/// CheckDoubleTy indicates that 'fptype' must be 'double'.<br>
> +static bool matchesFPLibFunctionSignature(const Function *F, unsigned NumParams,<br>
> + bool CheckDoubleTy) {<br>
> + FunctionType *FT = F->getFunctionType();<br>
> + if (FT->getNumParams() != NumParams)<br>
> + return false;<br>
> +<br>
> + // The return type must match what we're looking for.<br>
> + Type *RetTy = FT->getReturnType();<br>
> + if (CheckDoubleTy ? !RetTy->isDoubleTy() : !RetTy->isFloatingPointTy())<br>
> + return false;<br>
> +<br>
> + // Each parameter must match the return type, and therefore, match every other<br>
> + // parameter too.<br>
> + for (const Type *ParamTy : FT->params())<br>
> + if (ParamTy != RetTy)<br>
> + return false;<br>
> +<br>
> + return true;<br>
> +}<br>
> +<br>
> /// Shrink double -> float for unary functions like 'floor'.<br>
> static Value *optimizeUnaryDoubleFP(CallInst *CI, IRBuilder<> &B,<br>
> bool CheckRetType) {<br>
> Function *Callee = CI->getCalledFunction();<br>
> - FunctionType *FT = Callee->getFunctionType();<br>
> - if (FT->getNumParams() != 1 || !FT->getReturnType()->isDoubleTy() ||<br>
> - !FT->getParamType(0)->isDoubleTy())<br>
> + if (!matchesFPLibFunctionSignature(Callee, 1, true))<br>
> return nullptr;<br>
><br>
> if (CheckRetType) {<br>
> @@ -997,12 +1018,7 @@ static Value *optimizeUnaryDoubleFP(Call<br>
> /// Shrink double -> float for binary functions like 'fmin/fmax'.<br>
> static Value *optimizeBinaryDoubleFP(CallInst *CI, IRBuilder<> &B) {<br>
> Function *Callee = CI->getCalledFunction();<br>
> - FunctionType *FT = Callee->getFunctionType();<br>
> - // Just make sure this has 2 arguments of the same FP type, which match the<br>
> - // result type.<br>
> - if (FT->getNumParams() != 2 || FT->getReturnType() != FT->getParamType(0) ||<br>
> - FT->getParamType(0) != FT->getParamType(1) ||<br>
> - !FT->getParamType(0)->isFloatingPointTy())<br>
> + if (!matchesFPLibFunctionSignature(Callee, 2, true))<br>
> return nullptr;<br>
><br>
> // If this is something like 'fmin((double)floatval1, (double)floatval2)',<br>
><br>
> Modified: llvm/trunk/test/Transforms/InstCombine/double-float-shrink-1.ll<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/double-float-shrink-1.ll?rev=258428&r1=258427&r2=258428&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/double-float-shrink-1.ll?rev=258428&r1=258427&r2=258428&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/test/Transforms/InstCombine/double-float-shrink-1.ll (original)<br>
> +++ llvm/trunk/test/Transforms/InstCombine/double-float-shrink-1.ll Thu Jan 21 14:19:54 2016<br>
> @@ -364,6 +364,26 @@ define float @max1(float %a, float %b) {<br>
> ; CHECK-NEXT: ret<br>
> }<br>
><br>
> +; A function can have a name that matches a common libcall,<br>
> +; but with the wrong type(s). Let it be.<br>
> +<br>
> +define float @fake_fmin(float %a, float %b) {<br>
> + %c = fpext float %a to fp128<br>
> + %d = fpext float %b to fp128<br>
> + %e = call fp128 @fmin(fp128 %c, fp128 %d)<br>
> + %f = fptrunc fp128 %e to float<br>
> + ret float %f<br>
> +<br>
> +; CHECK-LABEL: fake_fmin(<br>
> +; CHECK-NEXT: %c = fpext float %a to fp128<br>
> +; CHECK-NEXT: %d = fpext float %b to fp128<br>
> +; CHECK-NEXT: %e = call fp128 @fmin(fp128 %c, fp128 %d)<br>
> +; CHECK-NEXT: %f = fptrunc fp128 %e to float<br>
> +; CHECK-NEXT: ret float %f<br>
> +}<br>
> +<br>
> +declare fp128 @fmin(fp128, fp128) ; This is not the 'fmin' you're looking for.<br>
> +<br>
> declare double @fmax(double, double)<br>
><br>
> declare double @tanh(double)<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="javascript:;" onclick="_e(event, 'cvml', 'llvm-commits@lists.llvm.org')">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote>