added support for Celu op #3139
Conversation
Signed-off-by: logeshwaranmcw <logeshwaran.elanchelian@multicorewareinc.com>
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Thank you for quickly creating a PR for Celu! It's almost done.
Could you add backend tests for Celu which checks if we get correct answers for Celu or not? You would add the tests in this file
https://github.com/onnx/onnx-mlir/blob/main/test/backend/inference_backend.py#L73. Please refer to Selu as an example. Test names for Celu are https://github.com/onnx/onnx-mlir/blob/main/test/backend/all_test_names.txt#L231.
To run the backend test locally, you can invoke: make check-onnx-backend
|
|
||
| template <> | ||
| GenOpMix getGenOpMix<ONNXCeluOp>(Type t, Operation *op) { | ||
| return {{GenericOps::ArithmeticGop, 1}}; |
There was a problem hiding this comment.
We should count all types of operations, which helps generate the best SIMD code. Could you take a look at operation types here and update this? Thanks!
There was a problem hiding this comment.
Sorry for not noticing that earlier. I have now added all the operations @tungld .
There was a problem hiding this comment.
I don't see the added operations. I see 2 min/max {GenericOps::MinMaxGop, 2}, the add (already covered), but increase the count to reflect the -1, a mult, div, and exp: {GenericOps::DivGop, 1}, {GenericOps::MulGop, 1}, {GenericOps::ExpGop, 1}
There was a problem hiding this comment.
Hi @AlexandreEichenberger, sorry I had misplaced the operation in LeakyRelu. I've just corrected it.
There was a problem hiding this comment.
Thanks, looks good
| Value emitScalarOpFor<ONNXCeluOp>(ConversionPatternRewriter &rewriter, | ||
| Location loc, Operation *op, Type elementType, | ||
| ArrayRef<Value> scalarOperands) { | ||
| CheckIfCustomScalarOpIsSupported<ONNXCeluOp>(elementType); |
There was a problem hiding this comment.
It would be helpful if you put the equation for Celu here as a comment, say // celu(x) = max(0, x) + min(0, alpha * (exp(x/alpha) - 1))
There was a problem hiding this comment.
Added the comment
| @@ -2744,6 +2792,7 @@ void populateLoweringONNXElementwiseOpPattern(RewritePatternSet &patterns, | |||
| ONNXElementwiseBinaryOpLowering<mlir::ONNXPowOp>, | |||
| ONNXElementwiseUnaryOpLowering<mlir::ONNXReciprocalOp>, | |||
| ONNXElementwiseUnaryOpLowering<mlir::ONNXReluOp>, | |||
| ONNXElementwiseUnaryOpLowering<mlir::ONNXCeluOp>, | |||
There was a problem hiding this comment.
This list is ordered by the ONNX op name in the alphabetic order. So we should move ONNXCeluOp up.
There was a problem hiding this comment.
Moved the ONNX op name
|
@jenkins-droid test this please |
Signed-off-by: logeshwaranmcw <logeshwaran.elanchelian@multicorewareinc.com>
Added the backend test @tungld |
|
|
||
| template <> | ||
| GenOpMix getGenOpMix<ONNXCeluOp>(Type t, Operation *op) { | ||
| return {{GenericOps::ArithmeticGop, 1}}; |
There was a problem hiding this comment.
I don't see the added operations. I see 2 min/max {GenericOps::MinMaxGop, 2}, the add (already covered), but increase the count to reflect the -1, a mult, div, and exp: {GenericOps::DivGop, 1}, {GenericOps::MulGop, 1}, {GenericOps::ExpGop, 1}
|
@tungld thanks for spotting the missing model for SIMD. It does not appear to have been updated, I provided a detailed comment on how to update the model. I will keep track of that. Once that is done, do you want to go over PR again and approve it if all your other requests have been satisfied? |
Signed-off-by: logeshwaranmcw <logeshwaran.elanchelian@multicorewareinc.com>
|
Will merge once it passes the CIs @logeshwaranmcw, please respond if not ok with you. |
|
Jenkins Linux amd64 Build #16611 [push] added support for Celu o... started at 15:37 |
|
Jenkins Linux s390x Build #16613 [push] added support for Celu o... started at 16:37 |
|
Jenkins Linux ppc64le Build #15556 [push] added support for Celu o... started at 16:37 |
|
Jenkins Linux amd64 Build #16611 [push] added support for Celu o... passed after 1 hr 23 min |
|
Jenkins Linux s390x Build #16613 [push] added support for Celu o... passed after 1 hr 25 min |
It's completely fine, @AlexandreEichenberger. Thank you both, @AlexandreEichenberger and @tungld, for the review comments |
|
Thanks for contributing, looking forward to many more :-) |
Signed-off-by: logeshwaranmcw <logeshwaran.elanchelian@multicorewareinc.com> Co-authored-by: Alexandre Eichenberger <alexe@us.ibm.com>
Added support for Celu op in the ONNX to krnl and also added the test case in Elementwise_with_canonicalize.mlir and Elementwise_with_canonicalize_O3.mlir
closes #3137