[WIP][InstCombine] Missed optimization in math expression: squashing sin(asin), cos(acos)

[Quolyk]

Summary

Motivation: https://bugs.llvm.org/show_bug.cgi?id=35603. I have failing test on functions cos_acos_fast and sin_asin_fast. Specifically

%call = call fast double @acos(double %a)
%call = call fast double @asin(double %a)

are not erased after transform, however %call instruction is never used. I'm looking for help, as I do not know why it behaves this way.

[efriedma]

are not erased after transform, however %call instruction is never used.

asin can set errno, which is a side-effect. For "fast" calls we should probably treat them as dead anyway (in llvm::isMathLibCallNoop), but the issue generally doesn't come up because clang (incorrectly) marks math library calls readnone in fast-math mode.

[hfinkel]

lib/Transforms/InstCombine/InstCombineCalls.cpp

1273

Don't match function names like this directly (name matching should go through TLI). Something like this:

LibFunc Func;
auto *Callee = CallI->getCalledFunction();
if (!Callee || !TLI->getLibFunc(*Callee, Func) || !TLI->has(Func))
  return nullptr;

...

        (II.getIntrinsicID() == Intrinsic::sin && LibFunc == LibFunc_asin)) {

...

As it seems like we're likely to get a bunch of these, I recommend adding a matcher for TLI functions so that we can just do:

match(Src, m_LibCall<LibFunc_asin>(TLI, m_Value(V)))

[hfinkel]

In D41389#959975, @efriedma wrote:

are not erased after transform, however %call instruction is never used.

asin can set errno, which is a side-effect. For "fast" calls we should probably treat them as dead anyway (in llvm::isMathLibCallNoop), but the issue generally doesn't come up because clang (incorrectly) marks math library calls readnone in fast-math mode.

I've been guilty of saying this too, but it's not completely true. On systems (e.g., MacOS) where the math calls don't set errno ever, this is obviously fine. On systems (e.g., Linux) where the regular math calls might set errno, many of the calls never actually set errno under finite-math assumptions (e.g., sin and cos), and many others are redirected by /usr/include/bits/math-finite.h to __<name>_finite variants which don't. Thus, it's actually okay to set these as readnone (because the compiler (by setting FINITE_MATH_ONLY), system headers, and implementation conspire to make this correct).

In general, we should probably restrict these transformations which remove calls to cases where the calls are readnone to be safe.

lib/Transforms/InstCombine/InstCombineCalls.cpp

1273

And, also, you should check for LibFunc_asin_finite. And, also, to catch non-double-precision library calls, also asinf, asinf_finite, asinl, asinl_finite. Please add tests for these.

lib/Transforms/InstCombine/InstCombineCalls.cpp

if (C1 && C1->isInfinity()) {
       // fmax(x, inf) -> inf
       if (!C1->isNegative())
         return Arg1;
     }
   }
   return nullptr;
 }
 
+static Instruction *simplifyCosSin(IntrinsicInst &II, InstCombiner &IC) {
+  Value *SrcSrc;
+  Value *Src = II.getArgOperand(0);
+  if (II.getIntrinsicID() == Intrinsic::cos ||
+      II.getIntrinsicID() == Intrinsic::amdgcn_cos) {
+    if (match(Src, m_FNeg(m_Value(SrcSrc))) ||
+        match(Src, m_Intrinsic<Intrinsic::fabs>(m_Value(SrcSrc)))) {
+      // cos(-x) -> cos(x)
+      // cos(fabs(x)) -> cos(x)
+      II.setArgOperand(0, SrcSrc);
+      return &II;
+    }
+  }
+
+  if (!II.isFast())
+    return nullptr;
+
+  if (CallInst *CallI = dyn_cast<CallInst>(Src)) {
+    StringRef Name = CallI->getCalledFunction()->getName();
+    if ((II.getIntrinsicID() == Intrinsic::cos && Name == "acos") ||
+        (II.getIntrinsicID() == Intrinsic::sin && Name == "asin")) {
+      return IC.replaceInstUsesWith(II, CallI->getArgOperand(0));
+    }
+  }
+
+  return nullptr;
+}
+
 static bool maskIsAllOneOrUndef(Value *Mask) {
   auto *ConstMask = dyn_cast<Constant>(Mask);
   if (!ConstMask)
     return false;
   if (ConstMask->isAllOnesValue() || isa<UndefValue>(ConstMask))
     return true;
   for (unsigned I = 0, E = ConstMask->getType()->getVectorNumElements(); I != E;
        ++I) {
if (match(II->getArgOperand(0), m_FPExt(m_Value(ExtSrc))) &&
       NewFabs->copyFastMathFlags(II);
       NewFabs->takeName(II);
       return new FPExtInst(NewFabs, II->getType());
     }
 
     break;
   }
   case Intrinsic::cos:
-  case Intrinsic::amdgcn_cos: {
-    Value *SrcSrc;
-    Value *Src = II->getArgOperand(0);
-    if (match(Src, m_FNeg(m_Value(SrcSrc))) ||
-        match(Src, m_Intrinsic<Intrinsic::fabs>(m_Value(SrcSrc)))) {
-      // cos(-x) -> cos(x)
-      // cos(fabs(x)) -> cos(x)
-      II->setArgOperand(0, SrcSrc);
-      return II;
-    }
-
+  case Intrinsic::amdgcn_cos:
+  case Intrinsic::sin:
+  case Intrinsic::amdgcn_sin:
+    if (Instruction *I = simplifyCosSin(*II, *this))
+      return I;
     break;
-  }
+
   case Intrinsic::ppc_altivec_lvx:
   case Intrinsic::ppc_altivec_lvxl:
     // Turn PPC lvx -> load if the pointer is known aligned.
     if (getOrEnforceKnownAlignment(II->getArgOperand(0), 16, DL, II, &AC,
                                    &DT) >= 16) {
       Value *Ptr = Builder.CreateBitCast(II->getArgOperand(0),
                                          PointerType::getUnqual(II->getType()));
       return new LoadInst(Ptr);

test/Transforms/InstCombine/cos-intrinsic.ll

 ; RUN: opt < %s -instcombine -S | FileCheck %s
 ; This test makes sure that the undef is propagated for the cos instrinsic
 
 declare double    @llvm.cos.f64(double %Val)
 declare float     @llvm.cos.f32(float %Val)
 declare <2 x float> @llvm.cos.v2f32(<2 x float> %Val)
 
 declare float @llvm.fabs.f32(float %Val)
 declare <2 x float> @llvm.fabs.v2f32(<2 x float> %Val)
 
+declare double @acos(double %Val)
+
 ; Function Attrs: nounwind readnone
 define double @test1() {
 ; CHECK-LABEL: define double @test1(
 ; CHECK-NEXT: ret double 0.000000e+00
   %1 = call double @llvm.cos.f64(double undef)
   ret double %1
 }
 

 ; CHECK-NEXT: %cos = call <2 x float> @llvm.cos.v2f32(<2 x float> %x.fabs.fneg)
 ; CHECK-NEXT: ret <2 x float> %cos
 define <2 x float> @cos_fabs_fneg_v2f32(<2 x float> %x) {
   %x.fabs = call <2 x float> @llvm.fabs.v2f32(<2 x float> %x)
   %x.fabs.fneg = fsub <2 x float> <float -0.0, float -0.0>, %x.fabs
   %cos = call <2 x float> @llvm.cos.v2f32(<2 x float> %x.fabs.fneg)
   ret <2 x float> %cos
 }
+
+; CHECK-LABEL: @cos_acos(
+; CHECK-NEXT: %call = call double @acos(double %a)
+; CHECK-NEXT: %1 = call double @llvm.cos.f64(double %call)
+; CHECK-NEXT: ret double %1
+define double @cos_acos(double %a) {
+  %call = call double @acos(double %a)
+  %1 = call double @llvm.cos.f64(double %call)
+  ret double %1
+}
+
+; CHECK-LABEL: @cos_acos_fast(
+; CHECK-NEXT: ret double %a
+define double @cos_acos_fast(double %a) {
+  %call = call fast double @acos(double %a)
+  %1 = call fast double @llvm.cos.f64(double %call)
+  ret double %1
+}

test/Transforms/InstCombine/sin-intrinsic.ll

+; RUN: opt < %s -instcombine -S | FileCheck %s
+
+declare double @llvm.sin.f64(double)
+declare double @asin(double)
+
+; CHECK-LABEL: @sin_asin(
+; CHECK-NEXT: %call = call double @asin(double %a)
+; CHECK-NEXT: %1 = call double @llvm.sin.f64(double %call)
+; CHECK-NEXT: ret double %1
+define double @sin_asin(double %a) {
+  %call = call double @asin(double %a)
+  %1 = call double @llvm.sin.f64(double %call)
+  ret double %1
+}
+
+; CHECK-LABEL: @sin_asin_fast(
+; CHECK-NEXT: ret double %a
+define double @sin_asin_fast(double %a) {
+  %call = call fast double @asin(double %a)
+  %1 = call fast double @llvm.sin.f64(double %call)
+  ret double %1
+}