InstCombine: Fold cos(-x) -> cos(x)

[davide]

SimplifyLibCalls.cpp does a similar transformation (cos(-x) -> cos(x)).
I think we should have the logic for this simplifications in a single place, if possible, so maybe there we should unconditionally lower libc cos() -> llvm.cos and simplify here?

[arsenm]

In D28236#634125, @davide wrote:

SimplifyLibCalls.cpp does a similar transformation (cos(-x) -> cos(x)).
I think we should have the logic for this simplifications in a single place, if possible, so maybe there we should unconditionally lower libc cos() -> llvm.cos and simplify here?

I think that needs to be part of a larger change to fix the mess of math intrinsic and libcall handling. In the case of cos, I view the cos function as somewhat different from llvm.cos. I expect the intrinsic to be a possibly less precise machine instruction version of cos rather than a fully correct library call so I'm not sure it should always be replaced with the intrinsic.

[majnemer]

In D28236#634134, @arsenm wrote:

In D28236#634125, @davide wrote:

SimplifyLibCalls.cpp does a similar transformation (cos(-x) -> cos(x)).
I think we should have the logic for this simplifications in a single place, if possible, so maybe there we should unconditionally lower libc cos() -> llvm.cos and simplify here?

I think that needs to be part of a larger change to fix the mess of math intrinsic and libcall handling. In the case of cos, I view the cos function as somewhat different from llvm.cos. I expect the intrinsic to be a possibly less precise machine instruction version of cos rather than a fully correct library call so I'm not sure it should always be replaced with the intrinsic.

This is not the case, it is supposed to behave exactly as the libcall: http://llvm.org/docs/LangRef.html#id439

[davide]

In D28236#634134, @arsenm wrote:

In D28236#634125, @davide wrote:

SimplifyLibCalls.cpp does a similar transformation (cos(-x) -> cos(x)).
I think we should have the logic for this simplifications in a single place, if possible, so maybe there we should unconditionally lower libc cos() -> llvm.cos and simplify here?

I think that needs to be part of a larger change to fix the mess of math intrinsic and libcall handling. In the case of cos, I view the cos function as somewhat different from llvm.cos. I expect the intrinsic to be a possibly less precise machine instruction version of cos rather than a fully correct library call so I'm not sure it should always be replaced with the intrinsic.

Not quite, as that would violate LangRef cos semantic. http://llvm.org/docs/LangRef.html#llvm-cos-intrinsic

[arsenm]

In D28236#634154, @davide wrote:

In D28236#634134, @arsenm wrote:

In D28236#634125, @davide wrote:

SimplifyLibCalls.cpp does a similar transformation (cos(-x) -> cos(x)).
I think we should have the logic for this simplifications in a single place, if possible, so maybe there we should unconditionally lower libc cos() -> llvm.cos and simplify here?

I think that needs to be part of a larger change to fix the mess of math intrinsic and libcall handling. In the case of cos, I view the cos function as somewhat different from llvm.cos. I expect the intrinsic to be a possibly less precise machine instruction version of cos rather than a fully correct library call so I'm not sure it should always be replaced with the intrinsic.

Not quite, as that would violate LangRef cos semantic. http://llvm.org/docs/LangRef.html#llvm-cos-intrinsic

The C standard is uselessly underspecified. It doesn't have any constraints on the precision of math functions. IMO having the intrinsic be the same as the libcall is not useful. e.g. errno is not a meaningful concept on all platforms, and the intent of intrinsics is usually to map to a machine instruction.

[davide]

In D28236#634466, @arsenm wrote:

In D28236#634154, @davide wrote:

In D28236#634134, @arsenm wrote:

In D28236#634125, @davide wrote:

SimplifyLibCalls.cpp does a similar transformation (cos(-x) -> cos(x)).
I think we should have the logic for this simplifications in a single place, if possible, so maybe there we should unconditionally lower libc cos() -> llvm.cos and simplify here?

I think that needs to be part of a larger change to fix the mess of math intrinsic and libcall handling. In the case of cos, I view the cos function as somewhat different from llvm.cos. I expect the intrinsic to be a possibly less precise machine instruction version of cos rather than a fully correct library call so I'm not sure it should always be replaced with the intrinsic.

Not quite, as that would violate LangRef cos semantic. http://llvm.org/docs/LangRef.html#llvm-cos-intrinsic

The C standard is uselessly underspecified. It doesn't have any constraints on the precision of math functions. IMO having the intrinsic be the same as the libcall is not useful. e.g. errno is not a meaningful concept on all platforms, and the intent of intrinsics is usually to map to a machine instruction.

In D28236#634466, @arsenm wrote:

In D28236#634154, @davide wrote:

In D28236#634134, @arsenm wrote:

In D28236#634125, @davide wrote:

SimplifyLibCalls.cpp does a similar transformation (cos(-x) -> cos(x)).
I think we should have the logic for this simplifications in a single place, if possible, so maybe there we should unconditionally lower libc cos() -> llvm.cos and simplify here?

I think that needs to be part of a larger change to fix the mess of math intrinsic and libcall handling. In the case of cos, I view the cos function as somewhat different from llvm.cos. I expect the intrinsic to be a possibly less precise machine instruction version of cos rather than a fully correct library call so I'm not sure it should always be replaced with the intrinsic.

Not quite, as that would violate LangRef cos semantic. http://llvm.org/docs/LangRef.html#llvm-cos-intrinsic

The C standard is uselessly underspecified. It doesn't have any constraints on the precision of math functions. IMO having the intrinsic be the same as the libcall is not useful. e.g. errno is not a meaningful concept on all platforms, and the intent of intrinsics is usually to map to a machine instruction.

I don't necessarily disagree, but I think that changing the semantic of cos would require a much wider discussion on llvm-dev.

[arsenm]

In D28236#634597, @davide wrote:

In D28236#634466, @arsenm wrote:

In D28236#634154, @davide wrote:

In D28236#634134, @arsenm wrote:

In D28236#634125, @davide wrote:

SimplifyLibCalls.cpp does a similar transformation (cos(-x) -> cos(x)).
I think we should have the logic for this simplifications in a single place, if possible, so maybe there we should unconditionally lower libc cos() -> llvm.cos and simplify here?

I think that needs to be part of a larger change to fix the mess of math intrinsic and libcall handling. In the case of cos, I view the cos function as somewhat different from llvm.cos. I expect the intrinsic to be a possibly less precise machine instruction version of cos rather than a fully correct library call so I'm not sure it should always be replaced with the intrinsic.

Not quite, as that would violate LangRef cos semantic. http://llvm.org/docs/LangRef.html#llvm-cos-intrinsic

The C standard is uselessly underspecified. It doesn't have any constraints on the precision of math functions. IMO having the intrinsic be the same as the libcall is not useful. e.g. errno is not a meaningful concept on all platforms, and the intent of intrinsics is usually to map to a machine instruction.

In D28236#634466, @arsenm wrote:

In D28236#634154, @davide wrote:

In D28236#634134, @arsenm wrote:

In D28236#634125, @davide wrote:

SimplifyLibCalls.cpp does a similar transformation (cos(-x) -> cos(x)).
I think we should have the logic for this simplifications in a single place, if possible, so maybe there we should unconditionally lower libc cos() -> llvm.cos and simplify here?

I think that needs to be part of a larger change to fix the mess of math intrinsic and libcall handling. In the case of cos, I view the cos function as somewhat different from llvm.cos. I expect the intrinsic to be a possibly less precise machine instruction version of cos rather than a fully correct library call so I'm not sure it should always be replaced with the intrinsic.

Not quite, as that would violate LangRef cos semantic. http://llvm.org/docs/LangRef.html#llvm-cos-intrinsic

The C standard is uselessly underspecified. It doesn't have any constraints on the precision of math functions. IMO having the intrinsic be the same as the libcall is not useful. e.g. errno is not a meaningful concept on all platforms, and the intent of intrinsics is usually to map to a machine instruction.

I don't necessarily disagree, but I think that changing the semantic of cos would require a much wider discussion on llvm-dev.

Getting back on topic, is there any objection to doing this optimization for the intrinsic (and for llvm.amdgcn.cos) separately here?

[davide]

In D28236#635393, @arsenm wrote:

Getting back on topic, is there any objection to doing this optimization for the intrinsic (and for llvm.amdgcn.cos) separately here?

I think this is fine but if we go this route the code in SimplifyLibCalls should be removed and we should always lower there to @llvm.cos (as we do already for other intrinsics, FWIW, e.g. @llvm.fls)

[davide]

lib/Transforms/InstCombine/InstCombineCalls.cpp

1641

While here, I also noticed the amdgcn_cos is not documented in LangRef, maybe it could be (or somewhere else)

[arsenm]

lib/Transforms/InstCombine/InstCombineCalls.cpp

1641

Do we have formal documentation for target specific intrinsics? I thought only generic intrinsics belonged in the LangRef

[davide]

lib/Transforms/InstCombine/InstCombineCalls.cpp

1643–1649

Please match against fabs as well here and update the tests.

[arsenm]

Also handle fabs.

[davide]

LGTM.

[arsenm]

r291022

lib/Transforms/InstCombine/InstCombineCalls.cpp

if (match(II->getArgOperand(0),
               m_Select(m_Value(Cond), m_Constant(LHS), m_Constant(RHS)))) {
       CallInst *Call0 = Builder->CreateCall(II->getCalledFunction(), {LHS});
       CallInst *Call1 = Builder->CreateCall(II->getCalledFunction(), {RHS});
       return SelectInst::Create(Cond, Call0, Call1);
     }
 
     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;
+    }
+
+    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/amdgcn-intrinsics.ll

 }
 
 ; CHECK-LABEL: @test_constant_class_snan_test_pinf_f64(
 ; CHECK: ret i1 false
 define i1 @test_constant_class_snan_test_pinf_f64() nounwind {
   %val = call i1 @llvm.amdgcn.class.f64(double 0x7FF0000000000001, i32 512)
   ret i1 %val
 }
+
+; --------------------------------------------------------------------
+; llvm.amdgcn.cos
+; --------------------------------------------------------------------
+declare float @llvm.amdgcn.cos.f32(float) nounwind readnone
+declare float @llvm.fabs.f32(float) nounwind readnone
+
+; CHECK-LABEL: @cos_fneg_f32(
+; CHECK: %cos = call float @llvm.amdgcn.cos.f32(float %x)
+; CHECK-NEXT: ret float %cos
+define float @cos_fneg_f32(float %x) {
+  %x.fneg = fsub float -0.0, %x
+  %cos = call float @llvm.amdgcn.cos.f32(float %x.fneg)
+  ret float %cos
+}
+
+; CHECK-LABEL: @cos_fabs_f32(
+; CHECK-NEXT: %cos = call float @llvm.amdgcn.cos.f32(float %x)
+; CHECK-NEXT: ret float %cos
+define float @cos_fabs_f32(float %x) {
+  %x.fabs = call float @llvm.fabs.f32(float %x)
+  %cos = call float @llvm.amdgcn.cos.f32(float %x.fabs)
+  ret float %cos
+}
+
+; CHECK-LABEL: @cos_fabs_fneg_f32(
+; CHECK-NEXT: %cos = call float @llvm.amdgcn.cos.f32(float %x)
+; CHECK-NEXT: ret float %cos
+define float @cos_fabs_fneg_f32(float %x) {
+  %x.fabs = call float @llvm.fabs.f32(float %x)
+  %x.fabs.fneg = fsub float -0.0, %x.fabs
+  %cos = call float @llvm.amdgcn.cos.f32(float %x.fabs.fneg)
+  ret float %cos
+}

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)
 
 ; 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
 }
 
 
 ; Function Attrs: nounwind readnone
 define float @test2(float %d) {
 ; CHECK-LABEL: define float @test2(
 ; CHECK-NEXT: %cosval = call float @llvm.cos.f32(float %d)
    %cosval   = call float @llvm.cos.f32(float %d)
    %cosval2  = call float @llvm.cos.f32(float undef)
    %fsum   = fadd float %cosval2, %cosval
    ret float %fsum
 ; CHECK-NEXT: %fsum
 ; CHECK: ret float %fsum
 }
+
+; CHECK-LABEL: @cos_fneg_f32(
+; CHECK: %cos = call float @llvm.cos.f32(float %x)
+; CHECK-NEXT: ret float %cos
+define float @cos_fneg_f32(float %x) {
+  %x.fneg = fsub float -0.0, %x
+  %cos = call float @llvm.cos.f32(float %x.fneg)
+  ret float %cos
+}
+
+; FIXME: m_FNeg() doesn't handle vectors
+; CHECK-LABEL: @cos_fneg_v2f32(
+; CHECK: %x.fneg = fsub <2 x float> <float -0.000000e+00, float -0.000000e+00>, %x
+; CHECK-NEXT: %cos = call <2 x float> @llvm.cos.v2f32(<2 x float> %x.fneg)
+; CHECK-NEXT: ret <2 x float> %cos
+define <2 x float> @cos_fneg_v2f32(<2 x float> %x) {
+  %x.fneg = fsub <2 x float> <float -0.0, float -0.0>, %x
+  %cos = call <2 x float> @llvm.cos.v2f32(<2 x float> %x.fneg)
+  ret <2 x float> %cos
+}
+
+; CHECK-LABEL: @cos_fabs_f32(
+; CHECK-NEXT: %cos = call float @llvm.cos.f32(float %x)
+; CHECK-NEXT: ret float %cos
+define float @cos_fabs_f32(float %x) {
+  %x.fabs = call float @llvm.fabs.f32(float %x)
+  %cos = call float @llvm.cos.f32(float %x.fabs)
+  ret float %cos
+}
+
+; CHECK-LABEL: @cos_fabs_fneg_f32(
+; CHECK: %cos = call float @llvm.cos.f32(float %x)
+; CHECK-NEXT: ret float %cos
+define float @cos_fabs_fneg_f32(float %x) {
+  %x.fabs = call float @llvm.fabs.f32(float %x)
+  %x.fabs.fneg = fsub float -0.0, %x.fabs
+  %cos = call float @llvm.cos.f32(float %x.fabs.fneg)
+  ret float %cos
+}
+
+; CHECK-LABEL: @cos_fabs_fneg_v2f32(
+; CHECK: %x.fabs = call <2 x float> @llvm.fabs.v2f32(<2 x float> %x)
+; CHECK-NEXT: %x.fabs.fneg = fsub <2 x float> <float -0.000000e+00, float -0.000000e+00>, %x.fabs
+; 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
+}