SimplifyLibCalls: Remove incorrect optimization of fabs

[arsenm]

Summary

fabs(x * x) is not generally safe to assume x is positive if x is a NaN.
This is also less general than it could be, so this will be replaced
with a transformation on the intrinsic.

[efriedma]

You might want to put a comment in the tests briefly noting the problem with this transformation.

I think this transform is safe if the @llvm.fabs call is marked nnan, but I'm sure you already have some plan for that.

[majnemer]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1200

Couldn't this stay if the fmul is marked as nnan?

[arsenm]

lib/Transforms/Utils/SimplifyLibCalls.cpp

1200

Yes, but I want to move this to an optimization on the intrinsic

[arsenm]

r291359

lib/Transforms/Utils/SimplifyLibCalls.cpp

if (LdExpArg) {
       return CI;
     }
   }
   return Ret;
 }
 
 Value *LibCallSimplifier::optimizeFabs(CallInst *CI, IRBuilder<> &B) {
   Function *Callee = CI->getCalledFunction();
-  Value *Ret = nullptr;
   StringRef Name = Callee->getName();
   if (Name == "fabs" && hasFloatVersion(Name))
-    Ret = optimizeUnaryDoubleFP(CI, B, false);
+    return optimizeUnaryDoubleFP(CI, B, false);
 
-  Value *Op = CI->getArgOperand(0);
-  if (Instruction *I = dyn_cast<Instruction>(Op)) {
-    // Fold fabs(x * x) -> x * x; any squared FP value must already be positive.
-    if (I->getOpcode() == Instruction::FMul)
-      if (I->getOperand(0) == I->getOperand(1))
-        return Op;
-  }
-  return Ret;
+  return nullptr;
 }
 
 Value *LibCallSimplifier::optimizeFMinFMax(CallInst *CI, IRBuilder<> &B) {
   Function *Callee = CI->getCalledFunction();
   // If we can shrink the call to a float function rather than a double
   // function, do that first.
   StringRef Name = Callee->getName();
   if ((Name == "fmin" || Name == "fmax") && hasFloatVersion(Name))

test/Transforms/InstCombine/fabs.ll

 ; RUN: opt < %s -instcombine -S | FileCheck %s
 
 ; Make sure all library calls are eliminated when the input is known positive.
 
 declare float @fabsf(float)
 declare double @fabs(double)
 declare fp128 @fabsl(fp128)
 
 define float @square_fabs_call_f32(float %x) {
   %mul = fmul float %x, %x
   %fabsf = tail call float @fabsf(float %mul)
   ret float %fabsf
 
 ; CHECK-LABEL: square_fabs_call_f32(
 ; CHECK-NEXT: %mul = fmul float %x, %x
-; CHECK-NEXT: ret float %mul
+; CHECK-NEXT: %fabsf = tail call float @fabsf(float %mul)
+; CHECK-NEXT: ret float %fabsf
 }
 
 define double @square_fabs_call_f64(double %x) {
   %mul = fmul double %x, %x
   %fabs = tail call double @fabs(double %mul)
   ret double %fabs
 
 ; CHECK-LABEL: square_fabs_call_f64(
 ; CHECK-NEXT: %mul = fmul double %x, %x
-; CHECK-NEXT: ret double %mul
+; CHECK-NEXT: %fabs = tail call double @fabs(double %mul)
+; CHECK-NEXT: ret double %fabs
 }
 
 define fp128 @square_fabs_call_f128(fp128 %x) {
   %mul = fmul fp128 %x, %x
   %fabsl = tail call fp128 @fabsl(fp128 %mul)
   ret fp128 %fabsl
 
 ; CHECK-LABEL: square_fabs_call_f128(
 ; CHECK-NEXT: %mul = fmul fp128 %x, %x
-; CHECK-NEXT: ret fp128 %mul
+; CHECK-NEXT: %fabsl = tail call fp128 @fabsl(fp128 %mul)
+; CHECK-NEXT: ret fp128 %fabsl
 }
 
 ; Make sure all intrinsic calls are eliminated when the input is known positive.
 
 declare float @llvm.fabs.f32(float)
 declare double @llvm.fabs.f64(double)
 declare fp128 @llvm.fabs.f128(fp128)
 
 define float @square_fabs_intrinsic_f32(float %x) {
   %mul = fmul float %x, %x
   %fabsf = tail call float @llvm.fabs.f32(float %mul)
   ret float %fabsf
 
 ; CHECK-LABEL: square_fabs_intrinsic_f32(
 ; CHECK-NEXT: %mul = fmul float %x, %x
-; CHECK-NEXT: ret float %mul
+; CHECK-NEXT: %fabsf = tail call float @llvm.fabs.f32(float %mul)
+; CHECK-NEXT: ret float %fabsf
 }
 
 define double @square_fabs_intrinsic_f64(double %x) {
   %mul = fmul double %x, %x
   %fabs = tail call double @llvm.fabs.f64(double %mul)
   ret double %fabs
 
 ; CHECK-LABEL: square_fabs_intrinsic_f64(
 ; CHECK-NEXT: %mul = fmul double %x, %x
-; CHECK-NEXT: ret double %mul
+; CHECK-NEXT: %fabs = tail call double @llvm.fabs.f64(double %mul)
+; CHECK-NEXT: ret double %fabs
 }
 
 define fp128 @square_fabs_intrinsic_f128(fp128 %x) {
   %mul = fmul fp128 %x, %x
   %fabsl = tail call fp128 @llvm.fabs.f128(fp128 %mul)
   ret fp128 %fabsl
 
 ; CHECK-LABEL: square_fabs_intrinsic_f128(
 ; CHECK-NEXT: %mul = fmul fp128 %x, %x
-; CHECK-NEXT: ret fp128 %mul
+; CHECK-NEXT: %fabsl = tail call fp128 @llvm.fabs.f128(fp128 %mul)
+; CHECK-NEXT: ret fp128 %fabsl
 }
 
 ; Shrinking a library call to a smaller type should not be inhibited by nor inhibit the square optimization.
 
 define float @square_fabs_shrink_call1(float %x) {
   %ext = fpext float %x to double
   %sq = fmul double %ext, %ext
   %fabs = call double @fabs(double %sq)
   %trunc = fptrunc double %fabs to float
   ret float %trunc
 
 ; CHECK-LABEL: square_fabs_shrink_call1(
-; CHECK-NEXT: %trunc = fmul float %x, %x
+; CHECK-NEXT: %ext = fpext float %x to double
+; CHECK-NEXT: %sq = fmul double %ext, %ext
+; CHECK-NEXT: call double @fabs(double %sq)
+; CHECK-NEXT: %trunc = fptrunc double %fabs to float
 ; CHECK-NEXT: ret float %trunc
 }
 
 define float @square_fabs_shrink_call2(float %x) {
   %sq = fmul float %x, %x
   %ext = fpext float %sq to double
   %fabs = call double @fabs(double %ext)
   %trunc = fptrunc double %fabs to float
   ret float %trunc
 
 ; CHECK-LABEL: square_fabs_shrink_call2(
 ; CHECK-NEXT: %sq = fmul float %x, %x
-; CHECK-NEXT: ret float %sq
+; CHECK-NEXT: %fabsf = call float @fabsf(float %sq)
+; CHECK-NEXT: ret float %fabsf
 }
 
 ; CHECK-LABEL: @fabs_select_constant_negative_positive(
 ; CHECK: %fabs = select i1 %cmp, float 1.000000e+00, float 2.000000e+00
 ; CHECK-NEXT: ret float %fabs
 define float @fabs_select_constant_negative_positive(i32 %c) {
   %cmp = icmp eq i32 %c, 0
   %select = select i1 %cmp, float -1.0, float 2.0

test/Transforms/InstCombine/fast-math.ll

 define double @sqrt_intrinsic_arg_4th(double %x) {
   %mul = fmul fast double %x, %x
   %mul2 = fmul fast double %mul, %mul
   %sqrt = call fast double @llvm.sqrt.f64(double %mul2)
   ret double %sqrt
 
 ; CHECK-LABEL: sqrt_intrinsic_arg_4th(
 ; CHECK-NEXT: %mul = fmul fast double %x, %x
-; CHECK-NEXT: ret double %mul
+; CHECK-NEXT: %fabs = call fast double @llvm.fabs.f64(double %mul)
+; CHECK-NEXT: ret double %fabs
 }
 
 define double @sqrt_intrinsic_arg_5th(double %x) {
   %mul = fmul fast double %x, %x
   %mul2 = fmul fast double %mul, %x
   %mul3 = fmul fast double %mul2, %mul
   %sqrt = call fast double @llvm.sqrt.f64(double %mul3)
   ret double %sqrt
 
 ; CHECK-LABEL: sqrt_intrinsic_arg_5th(
 ; CHECK-NEXT: %mul = fmul fast double %x, %x
+; CHECK-NEXT: %fabs = call fast double @llvm.fabs.f64(double %mul)
 ; CHECK-NEXT: %sqrt1 = call fast double @llvm.sqrt.f64(double %x)
-; CHECK-NEXT: %1 = fmul fast double %mul, %sqrt1
+; CHECK-NEXT: %1 = fmul fast double %fabs, %sqrt1
 ; CHECK-NEXT: ret double %1
 }
 
 ; Check that square root calls have the same behavior.
 
 declare float @sqrtf(float)
 declare double @sqrt(double)
 declare fp128 @sqrtl(fp128)