[MLIR] Add sincos intrinsic to LLVM dialect #160561
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ashermancinelli
requested review from
clementval,
ftynse,
joker-eph and
vzakhari
|
@llvm/pr-subscribers-mlir-llvm @llvm/pr-subscribers-mlir Author: Asher Mancinelli (ashermancinelli) ChangesAdds llvm.intr.sincos operation using LLVM_TwoResultIntrOp in the mold of the frexp intrinsic. I don't see many intrinsics with verifiers, so please let me know if the verifier and/or the invalid MLIR tests don't look right. Full diff: https://github.com/llvm/llvm-project/pull/160561.diff 4 Files Affected:
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td
index e12b8ac84ba23..398388bd720be 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td
@@ -184,6 +184,15 @@ def LLVM_UMinOp : LLVM_BinarySameArgsIntrOpI<"umin">;
def LLVM_SinOp : LLVM_UnaryIntrOpF<"sin">;
def LLVM_CosOp : LLVM_UnaryIntrOpF<"cos">;
def LLVM_TanOp : LLVM_UnaryIntrOpF<"tan">;
+def LLVM_SincosOp : LLVM_TwoResultIntrOp<"sincos", [], [0],
+ [Pure], /*requiresFastmath=*/1> {
+ let arguments =
+ (ins LLVM_ScalarOrVectorOf<LLVM_AnyFloat>:$val,
+ DefaultValuedAttr<LLVM_FastmathFlagsAttr, "{}">:$fastmathFlags);
+ let assemblyFormat = "`(` operands `)` attr-dict `:` "
+ "functional-type(operands, results)";
+ let hasVerifier = 1;
+}
def LLVM_ASinOp : LLVM_UnaryIntrOpF<"asin">;
def LLVM_ACosOp : LLVM_UnaryIntrOpF<"acos">;
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
index a3d5d25b96ec2..0d5e9e87070df 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -4085,6 +4085,24 @@ printIndirectBrOpSucessors(OpAsmPrinter &p, IndirectBrOp op, Type flagType,
p << "]";
}
+//===----------------------------------------------------------------------===//
+// SincosOp (intrinsic)
+//===----------------------------------------------------------------------===//
+
+LogicalResult LLVM::SincosOp::verify() {
+ auto operandType = getOperand().getType();
+ auto resultType = getResult().getType();
+ auto resultStructType =
+ mlir::dyn_cast<mlir::LLVM::LLVMStructType>(resultType);
+ if (!resultStructType || resultStructType.getBody().size() != 2 ||
+ resultStructType.getBody()[0] != operandType ||
+ resultStructType.getBody()[1] != operandType) {
+ return emitOpError("expected result type to be an homogeneous struct with "
+ "two elements matching the operand type");
+ }
+ return success();
+}
+
//===----------------------------------------------------------------------===//
// AssumeOp (intrinsic)
//===----------------------------------------------------------------------===//
diff --git a/mlir/test/Dialect/LLVMIR/invalid.mlir b/mlir/test/Dialect/LLVMIR/invalid.mlir
index 1adecf264e8f6..627abd0665d8c 100644
--- a/mlir/test/Dialect/LLVMIR/invalid.mlir
+++ b/mlir/test/Dialect/LLVMIR/invalid.mlir
@@ -2014,3 +2014,24 @@ llvm.mlir.alias external @alias_resolver : !llvm.ptr {
}
// expected-error@+1 {{'llvm.mlir.ifunc' op must have a function resolver}}
llvm.mlir.ifunc external @foo : !llvm.func<void (ptr, i32)>, !llvm.ptr @alias_resolver {dso_local}
+
+// -----
+
+llvm.func @invalid_sincos_nonhomogeneous_return_type(%f: f32) -> () {
+ // expected-error@+1 {{op expected result type to be an homogeneous struct with two elements matching the operand type}}
+ llvm.intr.sincos(%f) : (f32) -> !llvm.struct<(f32, f64)>
+}
+
+// -----
+
+llvm.func @invalid_sincos_non_struct_return_type(%f: f32) -> () {
+ // expected-error@+1 {{op expected result type to be an homogeneous struct with two elements matching the operand type}}
+ llvm.intr.sincos(%f) : (f32) -> f32
+}
+
+// -----
+
+llvm.func @invalid_sincos_gt_2_element_struct_return_type(%f: f32) -> () {
+ // expected-error@+1 {{op expected result type to be an homogeneous struct with two elements matching the operand type}}
+ llvm.intr.sincos(%f) : (f32) -> !llvm.struct<(f32, f32, f32)>
+}
diff --git a/mlir/test/Target/LLVMIR/llvmir-intrinsics.mlir b/mlir/test/Target/LLVMIR/llvmir-intrinsics.mlir
index cf3e129879d09..d63584e5e03ab 100644
--- a/mlir/test/Target/LLVMIR/llvmir-intrinsics.mlir
+++ b/mlir/test/Target/LLVMIR/llvmir-intrinsics.mlir
@@ -146,6 +146,11 @@ llvm.func @trig_test(%arg0: f32, %arg1: vector<8xf32>) {
llvm.intr.tan(%arg0) : (f32) -> f32
// CHECK: call <8 x float> @llvm.tan.v8f32
llvm.intr.tan(%arg1) : (vector<8xf32>) -> vector<8xf32>
+
+ // CHECK: call { float, float } @llvm.sincos.f32
+ llvm.intr.sincos(%arg0) : (f32) -> !llvm.struct<(f32, f32)>
+ // CHECK: call { <8 x float>, <8 x float> } @llvm.sincos.v8f32
+ llvm.intr.sincos(%arg1) : (vector<8xf32>) -> !llvm.struct<(vector<8xf32>, vector<8xf32>)>
llvm.return
}
@@ -1302,6 +1307,8 @@ llvm.func @experimental_constrained_fpext(%s: f32, %v: vector<4xf32>) {
// CHECK-DAG: declare <8 x float> @llvm.ceil.v8f32(<8 x float>) #0
// CHECK-DAG: declare float @llvm.cos.f32(float)
// CHECK-DAG: declare <8 x float> @llvm.cos.v8f32(<8 x float>) #0
+// CHECK-DAG: declare { float, float } @llvm.sincos.f32(float)
+// CHECK-DAG: declare { <8 x float>, <8 x float> } @llvm.sincos.v8f32(<8 x float>) #0
// CHECK-DAG: declare float @llvm.copysign.f32(float, float)
// CHECK-DAG: declare float @llvm.rint.f32(float)
// CHECK-DAG: declare double @llvm.rint.f64(double)
|
clementval
reviewed
Sep 24, 2025
There was a problem hiding this comment.
LGTM. Thanks @ashermancinelli for adding this missing intrinsic.
|
The verifier looks good to me. |
joker-eph
reviewed
Sep 24, 2025
Adds llvm.intr.sincos operation using LLVM_TwoResultIntrOp in the mold of the frexp intrinsic.
ashermancinelli
force-pushed
the
ajm/llvm-sincos
branch
from
1ee7201 to
b692b77
Compare
|
Thanks for the reviews! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/116/builds/18872 Here is the relevant piece of the build log for the reference |
|
My change is the only one in the blame-list, but the test appears to be unrelated. I'll be watching the subsequent builds. |
|
The subsequent build passed tests. |
ashermancinelli
added a commit
to ashermancinelli/llvm-project
that referenced
this pull request
Sep 25, 2025
Now that `sincos` is a supported intrinsic in the LLVM dialect (llvm#160561) we are able to add the corresponding operation in the math dialect. We have several benchmarks that use sine and cosine in hot-loops, and saving some calculations by performing sine and cosine together can benefit performance. We would like to have a way to represent sincos in the math dialect. Two parts I'm unsure about: * What do we think of the assembly format? `math.sincos %floatlike : f32 -> f32, f32` With a custom assembly format we could omit the `->` and everything after, but I couldn't get the ODS to do that. Open to suggestions. * I implement `getShapeForUnroll()` here, but where is the best place to test the unroller interfaces? I'll keep poking around after sending this out for review.
3 tasks
ashermancinelli
added a commit
that referenced
this pull request
Sep 30, 2025
Now that `sincos` is a supported intrinsic in the LLVM dialect (#160561) we are able to add the corresponding operation in the math dialect and add conversion patterns for LLVM and NVVM. We have several benchmarks that use sine and cosine in hot-loops, and saving some calculations by performing them together can benefit performance. We would like to have a way to represent sincos in the math dialect.
ashermancinelli
added a commit
to ashermancinelli/llvm-project
that referenced
this pull request
Sep 30, 2025
We see performance improvements from using sincos to reuse calculations in hot loops that compute sin() and cos() on the same operand. Add a pass to identify sin() and cos() calls in the same block with the same operand and fast-math flags, and fuse them into a sincos op. Follow-up to: * llvm#160561 * llvm#160772
ashermancinelli
added a commit
to ashermancinelli/llvm-project
that referenced
this pull request
Sep 30, 2025
We see performance improvements from using sincos to reuse calculations in hot loops that compute sin() and cos() on the same operand. Add a pass to identify sin() and cos() calls in the same block with the same operand and fast-math flags, and fuse them into a sincos op. Follow-up to: * llvm#160561 * llvm#160772
ashermancinelli
added a commit
that referenced
this pull request
Oct 1, 2025
We see performance improvements from using sincos to reuse calculations in hot loops that compute sin() and cos() of the same operand. Add a pass to identify sin() and cos() calls in the same block with the same operand and fast-math flags, and fuse them into a sincos op. Follow-up to: * #160561 * #160772
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Oct 3, 2025
Adds llvm.intr.sincos operation using LLVM_TwoResultIntrOp in the mold of the frexp intrinsic.
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Oct 3, 2025
Now that `sincos` is a supported intrinsic in the LLVM dialect (llvm#160561) we are able to add the corresponding operation in the math dialect and add conversion patterns for LLVM and NVVM. We have several benchmarks that use sine and cosine in hot-loops, and saving some calculations by performing them together can benefit performance. We would like to have a way to represent sincos in the math dialect.
mahesh-attarde
pushed a commit
to mahesh-attarde/llvm-project
that referenced
this pull request
Oct 3, 2025
We see performance improvements from using sincos to reuse calculations in hot loops that compute sin() and cos() of the same operand. Add a pass to identify sin() and cos() calls in the same block with the same operand and fast-math flags, and fuse them into a sincos op. Follow-up to: * llvm#160561 * llvm#160772
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds llvm.intr.sincos operation using LLVM_TwoResultIntrOp in the mold of the frexp intrinsic. I don't see many intrinsics with verifiers, so please let me know if the verifier and/or the invalid MLIR tests don't look right. Thanks in advance!