Skip to content
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

Jump to bottom

[Mhlo] BatchNorm & Concat #1539

Merged
merged 7 commits into from
Jul 20, 2022
Merged

[Mhlo] BatchNorm & Concat #1539

merged 7 commits into from
Jul 20, 2022

Conversation

chenchongsong
Copy link
Contributor

This PR serves the following purposes:

@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@chenchongsong
Copy link
Contributor Author

FYI @yaochengji @Connor-XY

@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

yaochengji
yaochengji reviewed Jul 14, 2022
View reviewed changes
test/mlir/conversion/onnx_to_mhlo/Tensor/Concat.mlir Outdated
// RUN: onnx-mlir-opt --convert-onnx-to-mhlo %s -split-input-file | FileCheck %s

// Test when output shape is unkown
func @test_concat_unknown_dims(%arg0 : tensor<5x5x1x32xf32>, %arg1 : tensor<5x5x3x32xf32>) -> tensor<?x?x?x?xf32> {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this unknown? The shape could be inferred?

What you want to demo is <5x5x?x32> , <5x5x?x32> -> <5x5x?x32> ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we call --shape-inference before --convert-onnx-to-mhlo? It depends on whether MHLO ops require ranked tensors not not.

@tungld
Copy link
Collaborator

tungld commented Jul 15, 2022

@jenkins-droid test this please

tungld
tungld reviewed Jul 15, 2022
View reviewed changes
test/mlir/conversion/onnx_to_mhlo/Tensor/Concat.mlir Outdated
// RUN: onnx-mlir-opt --convert-onnx-to-mhlo %s -split-input-file | FileCheck %s

// Test when output shape is unkown
func @test_concat_unknown_dims(%arg0 : tensor<5x5x1x32xf32>, %arg1 : tensor<5x5x3x32xf32>) -> tensor<?x?x?x?xf32> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we call --shape-inference before --convert-onnx-to-mhlo? It depends on whether MHLO ops require ranked tensors not not.

src/Conversion/ONNXToMhlo/Tensor/Concat.cpp Outdated
if (op->getNumResults() < 1) {
op->emitError() << "ONNXConcatOp Has No Output\n";
return failure();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this catched by the operation verifier?

src/Conversion/ONNXToMhlo/Tensor/Concat.cpp Outdated
return failure();
}
RankedTensorType resultType =
op->getResult(0).getType().dyn_cast_or_null<RankedTensorType>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI, we have isRankedShapedType in TypeUtilities` for this purpose. Please feel free to use it.

src/Conversion/ONNXToMhlo/Tensor/Concat.cpp
op->getResult(0).getType().dyn_cast_or_null<RankedTensorType>();
if (resultType == nullptr) {
op->emitError() << "Concat Output Is Not Ranked\n";
return failure();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not specific to this PR, I am a bit concerned about failures during rewrite patterns that can cause the whole compilation pipeline terminated. This would work if all ONNX ops can be lowered to MHLO. However, if an op does not satisfy the condition here, there is no a fallback path so that the op will be lowered to other dialects such as Krnl for CPU or ZHigh for NNPA.

One way to solve this is to use DynamicallyLegalOp in --convert--onnx-to-mhlo pass to selectively add operations into the conversion target. We did this for --convert-onnx-to-zhigh: https://github.com/onnx/onnx-mlir/blob/main/src/Accelerators/NNPA/Conversion/ONNXToZHigh/ONNXToZHigh.cpp#L323

FYI @AlexandreEichenberger

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment! Will discuss with co-workers about the fallback path for mhlo lowering. @yaochengji

Copy link
Member

Choose a reason for hiding this comment

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

@tungld Currently -convert-onnx-to-hlo uses applyPartialConversion and it doesn't make ONNX Dialect or any ONNX operations illegal. Therefore it is fine if there're some ONNX ops couldn't be converted to MHLO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@yaochengji I see. Thanks for clarifying that!

@chenchongsong
[Mhlo] BatchNorm & Concat
94d96cb 
Signed-off-by: chongsong.chen <chongsong.chen@bytedance.com>
@chenchongsong
[Mhlo] use 'result' instead of 'result->getResults()'
2cedfc1 
Signed-off-by: chongsong.chen <chongsong.chen@bytedance.com>
@chenchongsong
[Mhlo] fixes for code review
e3ed189 
Signed-off-by: chongsong.chen <chongsong.chen@bytedance.com>
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

1 similar comment
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

yaochengji
yaochengji reviewed Jul 18, 2022
View reviewed changes
src/Conversion/ONNXToMhlo/Tensor/Concat.cpp Outdated
assert(op->getNumResults() == 1 && "ONNXConcatOp shoule have 1 result");
Type resultType = op->getResult(0).getType();
if (!onnx_mlir::isRankedShapedType(resultType)) {
op->emitError() << "Concat Output Is Not Ranked\n";
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's an error. Suggest using llvm::dbgs() and DEBUG_TYPE here.

@chenchongsong
Fixes CMakeList & logging for debug
62e8096 
Signed-off-by: chongsong.chen <chongsong.chen@bytedance.com>
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@AlexandreEichenberger
Copy link
Collaborator

@chenchongsong merged with the latest before starting to make a run, hope its ok with you.

tungld
tungld approved these changes Jul 20, 2022
View reviewed changes
Copy link
Collaborator

@tungld tungld left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@tungld tungld merged commit 3a6db23 into onnx:main Jul 20, 2022
@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #6671 [push] [Mhlo] BatchNorm & Conca... started at 03:42

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #5783 [push] [Mhlo] BatchNorm & Conca... started at 04:45

@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #6687 [push] [Mhlo] BatchNorm & Conca... started at 04:42

@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #6687 [push] [Mhlo] BatchNorm & Conca... passed after 1 hr 43 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #5783 [push] [Mhlo] BatchNorm & Conca... passed after 1 hr 45 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #6671 [push] [Mhlo] BatchNorm & Conca... passed after 1 hr 59 min

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yaochengji yaochengji yaochengji left review comments

@tungld tungld tungld approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@chenchongsong @jenkins-droid @tungld @AlexandreEichenberger @yaochengji