[CIR][Lowering] Add MLIR lowering support for CIR sin operations #586
Conversation
GaoXiangYa
changed the title
bcardosolopes
changed the title
bcardosolopes
left a comment
bcardosolopes
left a comment
There was a problem hiding this comment.
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: } No newline at end of file |
There was a problem hiding this comment.
Nit: Please add the newline.
| } | ||
| }; | ||
|
|
||
| class CIRSinOpLowering : public mlir::OpConversionPattern<mlir::cir::SinOp> { |
There was a problem hiding this comment.
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.
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.
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. --------- Co-authored-by: Gao Xiang <gaoxiang@gaoxiang>
3 participants
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.