-
Notifications
You must be signed in to change notification settings - Fork 127
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.
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
[CIR][Lowering] Add MLIR lowering support for CIR sin operations #586
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! Missing just a merge conflict fix, once you handle that I'll land it.
@bcardosolopes Hello, I have dealt with conflict, and in the next pr, I will add cir.sqrt, cir.fabs, cif.floor, cir.ceil lowering mlir passes together |
// CHECK-NEXT: %{{.+}} = math.sin %[[C3]] : f64 | ||
// CHECK-NEXT: return | ||
// CHECK-NEXT: } | ||
// CHECK-NEXT: } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Please add the newline.
@@ -208,6 +209,18 @@ class CIRCosOpLowering : public mlir::OpConversionPattern<mlir::cir::CosOp> { | |||
} | |||
}; | |||
|
|||
class CIRSinOpLowering : public mlir::OpConversionPattern<mlir::cir::SinOp> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I suggest to introduce a template for the 1:1 lowerings from cir
to math
ops instead? Here, it would just be used by the existing cos
and new sin
pattern, but looking at #592, the boilerplate duplication would become quite significant otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right, we can use a template for the 1:1 lowerings from cir
to mlir math
ops, and we can also use the same template for the 1:1 lowerings from cir
to llvm math
ops. But the cir.rint
cir.shift
ops cannot use template, these ops should use the old way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using templates I'd rather leverage tablegen to do these. There's a similar idea introduced for LLVM lowering in #434 we could leverage, once that lands (I might commander that PR soon, but happy to let someone else take it) we could do similar for MLIR lowering.
…m#586) This PR add cir.sin lowering to MLIR math dialect. In the future, I will submit a PR to lowering cir.floor, cir.fabs and other operations to MLIR. --------- Co-authored-by: Gao Xiang <gaoxiang@gaoxiang>
…m#586) This PR add cir.sin lowering to MLIR math dialect. In the future, I will submit a PR to lowering cir.floor, cir.fabs and other operations to MLIR. --------- Co-authored-by: Gao Xiang <gaoxiang@gaoxiang>
…m#586) This PR add cir.sin lowering to MLIR math dialect. In the future, I will submit a PR to lowering cir.floor, cir.fabs and other operations to MLIR. --------- Co-authored-by: Gao Xiang <gaoxiang@gaoxiang>
This PR add cir.sin lowering to MLIR math dialect. In the future, I will submit a PR to lowering cir.floor, cir.fabs and other operations to MLIR. --------- Co-authored-by: Gao Xiang <gaoxiang@gaoxiang>
This PR add cir.sin lowering to MLIR math dialect. In the future, I will submit a PR to lowering cir.floor, cir.fabs and other operations to MLIR.