Skip to content

[InstCombine] Fold tan(x) * cos(x) => sin(x) #136319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

[InstCombine] Fold tan(x) * cos(x) => sin(x) #136319

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1f2fb83
[InstCombine] Pre-commit tests (NFC)
amordo Apr 18, 2025
f06b8aa
[InstCombine] Fold tan(x) * cos(x) => sin(x)
amordo Apr 18, 2025
3ce32a0
Apply recommendations
amordo Apr 19, 2025
52e45f7
Use m_Deferred instead m_Specific
amordo Apr 20, 2025
ac3ab9a
Add commuted and negative tests
amordo Apr 20, 2025
d23c71f
Preserve fpmath flag; add tests
amordo Apr 23, 2025
e39ab3f
Merge branch 'llvm:main' into fold_tan_mul_cos
amordo May 1, 2025
4715cb8
Check contract flag instead of reassoc
amordo May 1, 2025
afb912f
Merge branch 'main' into fold_tan_mul_cos
amordo May 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,6 +947,14 @@ Instruction *InstCombinerImpl::foldFMulReassoc(BinaryOperator &I) {
return BinaryOperator::CreateFMulFMF(XX, Y, &I);
}

// tan(X) * cos(X) -> sin(X)
if (match(&I,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reassoc/afn may be required, but I am not sure.

Copy link
Contributor

@arsenm arsenm Apr 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would fall under contract, and I expect it to be precision improving

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand correctly the instruction should only be checked for the contract flag?

The diff will be 9d090b2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like contract being used for this purpose. I think contract should be more predictable and tied to hardware behavior. Historically, we have used reassoc for algebraic transformations. If the user has enabled unsafe math optimizations broadly, this sort of optimization is what they'd expect, but if they've only enabled contract I don't think it is.

Copy link
Contributor

@arsenm arsenm Apr 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think contract should be more predictable and tied to hardware behavior

This should have no relation to hardware behavior whatsoever. Hardware dependent semantics are simply not useful.

This is also inline with the proposal to fix the definition for the contract flag: https://discourse.llvm.org/t/rfc-fast-math-flags-semantics-contract/84478

contract should not be limited to the FMA case. If that were the intended only case to handle, it should not be a fast math flag. It needs broader applicability, and should cover cases that may increase precision

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you are suggesting here is, IMO, a significant extension to the way the contract flag is interpreted

It's how the contract flag is already interpreted, see https://discourse.llvm.org/t/rfc-fast-math-flags-semantics-contract/84478

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other optimizations using it this way?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the backend we have some x / sqrt -> rsqrt contractions I wrote, I'd have to look at what other call optimizers are doing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to interpret the flag this way, the LangRef should be updated. I guess we can just deal with the fallout in clang if users complain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied changes in 4715cb8
Is it resolved and PR could be merged?

m_c_FMul(m_OneUse(m_Intrinsic<Intrinsic::tan>(m_Value(X))),
m_OneUse(m_Intrinsic<Intrinsic::cos>(m_Deferred(X)))))) {
Value *Sin = Builder.CreateUnaryIntrinsic(Intrinsic::sin, X, &I);
return replaceInstUsesWith(I, Sin);
}

return nullptr;
}

Expand Down
136 changes: 136 additions & 0 deletions llvm/test/Transforms/InstCombine/fmul-tan-cos.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
; RUN: opt -S -passes=instcombine < %s | FileCheck %s

define double @fmul_tan_cos(double %a) {
; CHECK-LABEL: define double @fmul_tan_cos(
; CHECK-SAME: double [[A:%.*]]) {
; CHECK-NEXT: [[TAN:%.*]] = call double @llvm.tan.f64(double [[A]])
; CHECK-NEXT: [[COS:%.*]] = call double @llvm.cos.f64(double [[A]])
; CHECK-NEXT: [[RES:%.*]] = fmul double [[TAN]], [[COS]]
; CHECK-NEXT: ret double [[RES]]
;
%tan = call double @llvm.tan.f64(double %a)
%cos = call double @llvm.cos.f64(double %a)
%res = fmul double %tan, %cos
ret double %res
}

define double @fmul_strict_tan_strict_cos_reassoc(double %a) {
; CHECK-LABEL: define double @fmul_strict_tan_strict_cos_reassoc(
; CHECK-SAME: double [[A:%.*]]) {
; CHECK-NEXT: [[TAN:%.*]] = call double @llvm.tan.f64(double [[A]])
; CHECK-NEXT: [[COS:%.*]] = call reassoc double @llvm.cos.f64(double [[A]])
; CHECK-NEXT: [[RES:%.*]] = fmul double [[TAN]], [[COS]]
; CHECK-NEXT: ret double [[RES]]
;
%tan = call double @llvm.tan.f64(double %a)
%cos = call reassoc double @llvm.cos.f64(double %a)
%res = fmul double %tan, %cos
ret double %res
}

define double @fmul_reassoc_tan_strict_cos_strict(double %a) {
; CHECK-LABEL: define double @fmul_reassoc_tan_strict_cos_strict(
; CHECK-SAME: double [[A:%.*]]) {
; CHECK-NEXT: [[RES:%.*]] = call reassoc double @llvm.sin.f64(double [[A]])
; CHECK-NEXT: ret double [[RES]]
;
%tan = call double @llvm.tan.f64(double %a)
%cos = call double @llvm.cos.f64(double %a)
%res = fmul reassoc double %tan, %cos
ret double %res
}

define double @fmul_reassoc_tan_reassoc_cos_strict(double %a) {
; CHECK-LABEL: define double @fmul_reassoc_tan_reassoc_cos_strict(
; CHECK-SAME: double [[A:%.*]]) {
; CHECK-NEXT: [[RES:%.*]] = call reassoc double @llvm.sin.f64(double [[A]])
; CHECK-NEXT: ret double [[RES]]
;
%tan = call reassoc double @llvm.tan.f64(double %a)
%cos = call double @llvm.cos.f64(double %a)
%res = fmul reassoc double %tan, %cos
ret double %res
}

define double @fmul_tan_cos_reassoc_multiple_uses(double %a) {
; CHECK-LABEL: define double @fmul_tan_cos_reassoc_multiple_uses(
; CHECK-SAME: double [[A:%.*]]) {
; CHECK-NEXT: [[TAN:%.*]] = call reassoc double @llvm.tan.f64(double [[A]])
; CHECK-NEXT: [[COS:%.*]] = call reassoc double @llvm.cos.f64(double [[A]])
; CHECK-NEXT: [[RES:%.*]] = fmul reassoc double [[TAN]], [[COS]]
; CHECK-NEXT: call void @use(double [[COS]])
; CHECK-NEXT: ret double [[RES]]
;
%tan = call reassoc double @llvm.tan.f64(double %a)
%cos = call reassoc double @llvm.cos.f64(double %a)
%res = fmul reassoc double %tan, %cos
call void @use(double %cos)
ret double %res
}

define double @fmul_tan_cos_reassoc(double %a) {
; CHECK-LABEL: define double @fmul_tan_cos_reassoc(
; CHECK-SAME: double [[A:%.*]]) {
; CHECK-NEXT: [[RES:%.*]] = call reassoc double @llvm.sin.f64(double [[A]])
; CHECK-NEXT: ret double [[RES]]
;
%tan = call reassoc double @llvm.tan.f64(double %a)
%cos = call reassoc double @llvm.cos.f64(double %a)
%res = fmul reassoc double %tan, %cos
ret double %res
}

define float @fmul_tanf_cosf_reassoc(float %a) {
; CHECK-LABEL: define float @fmul_tanf_cosf_reassoc(
; CHECK-SAME: float [[A:%.*]]) {
; CHECK-NEXT: [[RES:%.*]] = call reassoc float @llvm.sin.f32(float [[A]])
; CHECK-NEXT: ret float [[RES]]
;
%tan = call reassoc float @llvm.tan.f32(float %a)
%cos = call reassoc float @llvm.cos.f32(float %a)
%res = fmul reassoc float %tan, %cos
ret float %res
}

define fp128 @fmul_tanfp128_cosfp128_reassoc(fp128 %a) {
; CHECK-LABEL: define fp128 @fmul_tanfp128_cosfp128_reassoc(
; CHECK-SAME: fp128 [[A:%.*]]) {
; CHECK-NEXT: [[RES:%.*]] = call reassoc fp128 @llvm.sin.f128(fp128 [[A]])
; CHECK-NEXT: ret fp128 [[RES]]
;
%tan = call reassoc fp128 @llvm.tan.fp128(fp128 %a)
%cos = call reassoc fp128 @llvm.cos.fp128(fp128 %a)
%res = fmul reassoc fp128 %tan, %cos
ret fp128 %res
}

; commutativity
define double @commutativity_cos_tan(double %a) {
; CHECK-LABEL: define double @commutativity_cos_tan(
; CHECK-SAME: double [[A:%.*]]) {
; CHECK-NEXT: [[RES:%.*]] = call reassoc double @llvm.sin.f64(double [[A]])
; CHECK-NEXT: ret double [[RES]]
;
%cos = call reassoc double @llvm.cos.f64(double %a)
%tan = call reassoc double @llvm.tan.f64(double %a)
%res = fmul reassoc double %cos, %tan
ret double %res
}

; negative test with mismatched value
define double @tan_cos_value_mismatch(double %a, double %b) {
; CHECK-LABEL: define double @tan_cos_value_mismatch(
; CHECK-SAME: double [[A:%.*]], double [[B:%.*]]) {
; CHECK-NEXT: [[TAN:%.*]] = call reassoc double @llvm.tan.f64(double [[A]])
; CHECK-NEXT: [[COS:%.*]] = call reassoc double @llvm.cos.f64(double [[B]])
; CHECK-NEXT: [[RES:%.*]] = fmul reassoc double [[TAN]], [[COS]]
; CHECK-NEXT: ret double [[RES]]
;
%tan = call reassoc double @llvm.tan.f64(double %a)
%cos = call reassoc double @llvm.cos.f64(double %b)
%res = fmul reassoc double %tan, %cos
ret double %res
}

declare void @use(double)
Loading