[llvm] r258428 - [LibCallSimplifier] don't get fooled by a fake fmin()

[Sanjay Patel via llvm-commits]

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?

On Thursday, January 21, 2016, Hans Wennborg <hans at chromium.org> wrote:

> Sanjay, it was pointed out by Joerg that this should probably also be
> merged to 3.8?
>
> On Thu, Jan 21, 2016 at 12:19 PM, Sanjay Patel via llvm-commits
> <llvm-commits at lists.llvm.org <javascript:;>> wrote:
> > Author: spatel
> > Date: Thu Jan 21 14:19:54 2016
> > New Revision: 258428
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=258428&view=rev
> > Log:
> > [LibCallSimplifier] don't get fooled by a fake fmin()
> >
> > This is similar to the bug/fix:
> > https://llvm.org/bugs/show_bug.cgi?id=26211
> > http://reviews.llvm.org/rL258325
> >
> > The fmin() test case reveals another bug caused by sloppy
> > code duplication. It will crash without this patch because
> > fp128 is a valid floating-point type, but we would think
> > that we had matched a function that used doubles.
> >
> > The new helper function can be used to replace similar
> > checks that are used in several other places in this file.
> >
> >
> > Modified:
> >     llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp
> >     llvm/trunk/test/Transforms/InstCombine/double-float-shrink-1.ll
> >
> > Modified: llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp?rev=258428&r1=258427&r2=258428&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp (original)
> > +++ llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp Thu Jan 21
> 14:19:54 2016
> > @@ -953,13 +953,34 @@ static Value *valueHasFloatPrecision(Val
> >    return nullptr;
> >  }
> >
> > +/// Any floating-point library function that we're trying to simplify
> will have
> > +/// a signature of the form: fptype foo(fptype param1, fptype param2,
> ...).
> > +/// CheckDoubleTy indicates that 'fptype' must be 'double'.
> > +static bool matchesFPLibFunctionSignature(const Function *F, unsigned
> NumParams,
> > +                                          bool CheckDoubleTy) {
> > +  FunctionType *FT = F->getFunctionType();
> > +  if (FT->getNumParams() != NumParams)
> > +    return false;
> > +
> > +  // The return type must match what we're looking for.
> > +  Type *RetTy = FT->getReturnType();
> > +  if (CheckDoubleTy ? !RetTy->isDoubleTy() :
> !RetTy->isFloatingPointTy())
> > +    return false;
> > +
> > +  // Each parameter must match the return type, and therefore, match
> every other
> > +  // parameter too.
> > +  for (const Type *ParamTy : FT->params())
> > +    if (ParamTy != RetTy)
> > +      return false;
> > +
> > +  return true;
> > +}
> > +
> >  /// Shrink double -> float for unary functions like 'floor'.
> >  static Value *optimizeUnaryDoubleFP(CallInst *CI, IRBuilder<> &B,
> >                                      bool CheckRetType) {
> >    Function *Callee = CI->getCalledFunction();
> > -  FunctionType *FT = Callee->getFunctionType();
> > -  if (FT->getNumParams() != 1 || !FT->getReturnType()->isDoubleTy() ||
> > -      !FT->getParamType(0)->isDoubleTy())
> > +  if (!matchesFPLibFunctionSignature(Callee, 1, true))
> >      return nullptr;
> >
> >    if (CheckRetType) {
> > @@ -997,12 +1018,7 @@ static Value *optimizeUnaryDoubleFP(Call
> >  /// Shrink double -> float for binary functions like 'fmin/fmax'.
> >  static Value *optimizeBinaryDoubleFP(CallInst *CI, IRBuilder<> &B) {
> >    Function *Callee = CI->getCalledFunction();
> > -  FunctionType *FT = Callee->getFunctionType();
> > -  // Just make sure this has 2 arguments of the same FP type, which
> match the
> > -  // result type.
> > -  if (FT->getNumParams() != 2 || FT->getReturnType() !=
> FT->getParamType(0) ||
> > -      FT->getParamType(0) != FT->getParamType(1) ||
> > -      !FT->getParamType(0)->isFloatingPointTy())
> > +  if (!matchesFPLibFunctionSignature(Callee, 2, true))
> >      return nullptr;
> >
> >    // If this is something like 'fmin((double)floatval1,
> (double)floatval2)',
> >
> > Modified: llvm/trunk/test/Transforms/InstCombine/double-float-shrink-1.ll
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/double-float-shrink-1.ll?rev=258428&r1=258427&r2=258428&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/test/Transforms/InstCombine/double-float-shrink-1.ll
> (original)
> > +++ llvm/trunk/test/Transforms/InstCombine/double-float-shrink-1.ll Thu
> Jan 21 14:19:54 2016
> > @@ -364,6 +364,26 @@ define float @max1(float %a, float %b) {
> >  ; CHECK-NEXT:  ret
> >  }
> >
> > +; A function can have a name that matches a common libcall,
> > +; but with the wrong type(s). Let it be.
> > +
> > +define float @fake_fmin(float %a, float %b) {
> > +  %c = fpext float %a to fp128
> > +  %d = fpext float %b to fp128
> > +  %e = call fp128 @fmin(fp128 %c, fp128 %d)
> > +  %f = fptrunc fp128 %e to float
> > +  ret float %f
> > +
> > +; CHECK-LABEL: fake_fmin(
> > +; CHECK-NEXT:  %c = fpext float %a to fp128
> > +; CHECK-NEXT:  %d = fpext float %b to fp128
> > +; CHECK-NEXT:  %e = call fp128 @fmin(fp128 %c, fp128 %d)
> > +; CHECK-NEXT:  %f = fptrunc fp128 %e to float
> > +; CHECK-NEXT:  ret float %f
> > +}
> > +
> > +declare fp128 @fmin(fp128, fp128) ; This is not the 'fmin' you're
> looking for.
> > +
> >  declare double @fmax(double, double)
> >
> >  declare double @tanh(double)
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org <javascript:;>
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160121/a6c16b72/attachment.html>