Missed optimization in math expression: tan(a) * cos(a) == sin(a)

[zamazan4ik]

Bugzilla Link	35602
Version	trunk
OS	All
Blocks	#34959
CC	@hfinkel,@rotateright

Extended Description

clang(trunk) with '--std=c++17 -O3 -march=native -ffast-math' flags for this code:

#include <cmath>

double test(double a)
{
    return tan(a) * cos(a);
}

generates this assembly:

test(double): # @test(double)
  sub rsp, 24
  vmovsd qword ptr [rsp + 8], xmm0 # 8-byte Spill
  call tan
  vmovsd qword ptr [rsp + 16], xmm0 # 8-byte Spill
  vmovsd xmm0, qword ptr [rsp + 8] # 8-byte Reload
  call cos
  vmulsd xmm0, xmm0, qword ptr [rsp + 16] # 8-byte Folded Reload
  add rsp, 24
  ret

gcc(trunk) with '--std=c++17 -O3 -march=native -ffast-math' flags generates this:

test(double):
        jmp     sin

[hfinkel]

Could be in InstCombine.

[zamazan4ik]

See a lot of optimizations here: https://github.com/gcc-mirror/gcc/blob/07b69d3f1cd3dd8ebb0af1fbff95914daee477d2/gcc/match.pd

[llvmbot]

See https://reviews.llvm.org/D41283

[llvmbot]

Hi,
Is this still open?
I have seen that it is assigned for Adil. But since then no activity here.
If it is open, I will take it.
Thanks

[rotateright]

Hi,
Is this still open?
I have seen that it is assigned for Adil. But since then no activity here.
If it is open, I will take it.
Thanks

Yes, this is still open. I'm not sure if Dmitry is able to respond, but the last comment in D41283 suggested that we would add an llvm.tan* intrinsic corresponding to sin/cos. I still think that's the right 1st step to move this forward.

[llvmbot]

Sorry for the delay. I'm quit busy at the moment doing my primary job, but still working on this. In a couple of weeks I'm returning to this patch.

[RKSimon]

mentioned in issue #34959

[elhewaty]

Hi @zamazan4ik , I am new to LLVM. Can I work on this issue? please mention any resources that can help.

[llvmbot]

@llvm/issue-subscribers-good-first-issue

[RKSimon]

@elhewaty I say go for it - nobody has assigned it and the ticket has been quiet for long enough!

[EugeneZelenko]

Same should be done for hyperbolic equivalents.

[RKSimon]

It looks like the first task will be to add llvm.tan.* intrinsics - that was the first step identified on D41283 but it didn't go anywhere

Same if you're going to add the cosh/sinh/tanh equivalents

[Fare9]

Hi, I'm starting to work with LLVM, and if this issue can be solved without a deep understanding of all the optimizations I could try handling it. I have been checking the code, and maybe optimization can be included in: https://github.com/llvm/llvm-project/blob/2e999b7dd1934a44d38c3a753460f1e5a217e9a5/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp (InstCombineCalls.cpp). Since these are two calls that must be combined in only one.

For what @RKSimon said, does not exist a Intrinsic::tan?

[Fare9]

Sorry, previously I said that it would fit in InstCombineCalls.cpp, but probably it would fit better here:

llvm-project/llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp

Lines 1564 to 1587 in 2e999b7

	if (I.hasAllowReassoc() && Op0->hasOneUse() && Op1->hasOneUse()) {
	// sin(X) / cos(X) -> tan(X)
	// cos(X) / sin(X) -> 1/tan(X) (cotangent)
	Value *X;
	bool IsTan = match(Op0, m_Intrinsic<Intrinsic::sin>(m_Value(X))) &&
	match(Op1, m_Intrinsic<Intrinsic::cos>(m_Specific(X)));
	bool IsCot =
	!IsTan && match(Op0, m_Intrinsic<Intrinsic::cos>(m_Value(X))) &&
	match(Op1, m_Intrinsic<Intrinsic::sin>(m_Specific(X)));
	if ((IsTan || IsCot) && hasFloatFn(M, &TLI, I.getType(), LibFunc_tan,
	LibFunc_tanf, LibFunc_tanl)) {
	IRBuilder<> B(&I);
	IRBuilder<>::FastMathFlagGuard FMFGuard(B);
	B.setFastMathFlags(I.getFastMathFlags());
	AttributeList Attrs =
	cast<CallBase>(Op0)->getCalledFunction()->getAttributes();
	Value *Res = emitUnaryFloatFnCall(X, &TLI, LibFunc_tan, LibFunc_tanf,
	LibFunc_tanl, B, Attrs);
	if (IsCot)
	Res = B.CreateFDiv(ConstantFP::get(I.getType(), 1.0), Res);
	return replaceInstUsesWith(I, Res);
	}
	}

(InstCombineMulDivRem.cpp), there are already cases written cases of Intrinsic::cos and Intrinsic::sin. In any case I will wait for an answer if is needed to implement a Intrinsic::tan.

[RKSimon]

Issue #61617 noticed this similar fold was missing as well:

float f2(float x)
{
    return sinf(x) / tanf(x); // cosf(x)
}

[junaire]

Hi @RKSimon, I find this issue an excellent chance to learn different parts of LLVM. Do you think it's still the right direction to add llvm.tan.* intrinsic? If so I can work on this. Below is my roadmap:

1. Add llvm.tan.* intrinsic
2. Replace the old LibFunc_tan* pattern with the added intrinsic
3. Add new patterns for llvm.tan.* intrinsic

[RKSimon]

@junaire That plan looks good to me - (2) might involve extra work for correctly lowering tan intrinsics on specific targets, and you will have to do some minor work in the clang frontend to correctly match tan function calls.

[junaire]

I've put up https://reviews.llvm.org/D146905 to add llvm.tan.* intrinsic.

[junaire]

I've put up https://reviews.llvm.org/D146905 to add llvm.tan.* intrinsic.

The patch is abandoned due to some objections about adding more fp intrinsics from some reviewers.

[junaire]

Looks like we added a pattern match for LibFunc a while ago (cb238de) but it was reverted in (7284d44). The reason is "it introduced a layering violation by adding a dependency on IR to Analysis." Unfortunately, the author gave up his work on this and I wonder if it's possible to bring it back? If so, any insight about how to fix the violation and reland the patch?

CC @RKSimon @rotateright @iliya-diyachkov @topperc

[RKSimon]

That's a shame, I'd have gone with the llvm.tan.* approach, but I can understand if people who know the code better worry about diverging intrinsics / LibFunc implementations.

I can't think of a way to avoid the TLI dependency but you might be able to add a TargetPatternMatch.h header somewhere inside Analysis that you can wrap PatternMatch.h and add the equivalent TLI pattern match handling to - and then SimplifyLibCalls includes that instead of PatternMatch.h directly?

[junaire]

I don't know but just some random thought - Is it possible to canonicalize tan to llvm.sin / llvm.cos? At least with fast-math enabled. If so all optimization should be applied automatically.

[amordo]

Do I understand correctly the proof is not applicable here because of unsupported trigonometry intrinsics in alive2?
https://llvm.org/docs/InstCombineContributorGuide.html#proofs doesn't mention such case.

[amordo]

@junaire, are you still working on it or I could proceed with that?

[junaire]

@junaire, are you still working on it or I could proceed with that?

no i dont, i think you should talk to some maintainers about how we can handle this, probably start an rfc or something. i remember there was some disagreement about the patch and eventually i gave up...

[farzonl]

@junaire RFC was approved about a year ago. https://discourse.llvm.org/t/rfc-all-the-math-intrinsics/78294

@amordo Really excited to see people use the new trig intrinsics for inst combines.