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.

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

Decompose DepthToSpace operator. #910

Closed
wants to merge 3 commits into from
Closed

Conversation

etiotto
Copy link
Collaborator

@etiotto etiotto commented Oct 12, 2021

This PR decomposes the ONNX DepthToSpace operator into reshape+transpose+reshape as documented by https://github.com/onnx/onnx/blob/master/docs/Operators.md#DepthToSpace

Signed-off-by: Ettore Tiotto <[email protected]>
@tungld
Copy link
Collaborator

tungld commented Oct 13, 2021

@etiotto It seems DepthToSpace needs input shape to be rewritten. In such a case, src/Dialect/ONNX/Rewrite.td would be more suitable because when Decompose.cpp is called, tensors may not be unranked.

Could you please enable end-to-end tests for DepthToSpace in test/backend/test.py also? Thanks.

@etiotto
Copy link
Collaborator Author

etiotto commented Oct 14, 2021

@tungld thanks for the code review. I have implemented the canonicalization code in src/Dialect/ONNX/Rewrite.td as suggested and added end-toend tests in test/backend/test.py.
Can you please take another look?

@tungld
Copy link
Collaborator

tungld commented Oct 14, 2021

@etiotto make check-onnx-backend-dynamic failed, in case of dynamic tests, B, C, H, W will be -1, you should handle them too.

Copy link
Collaborator

@AlexandreEichenberger AlexandreEichenberger left a comment

Choose a reason for hiding this comment

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

Please comment on the source of all the changes in the ONNXOps.td.inc, as it is a auto-generated file.

@@ -821,7 +821,7 @@ def ONNXConvTransposeOp:ONNX_Op<"ConvTranspose",
"output_shape can also be explicitly specified in which case pads values are auto generated using these equations:"
""
" total_padding[i] = stride[i] * (input_size[i] - 1) + output_padding[i] + ((kernel_shape[i] - 1) * dilations[i] + 1) - output_shape[i]"
" If (auto_pads != SAME_UPPER): pads[start_i] = total_padding[i]/2; pads[end_i] = total_padding[i] - (total_padding[i]/2)"
" If (auto_pads == SAME_UPPER): pads[start_i] = total_padding[i]/2; pads[end_i] = total_padding[i] - (total_padding[i]/2)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you comment on the source of all these text changes in the ONNXOPS.td.inc?

@@ -353,7 +353,7 @@
OpsWithCanonicalizer = ['Add', 'Constant', 'Identity', 'Cast', 'Transpose',
'Dropout', 'Shape', 'Size', 'GlobalAveragePool',
'GlobalMaxPool', 'SqueezeV11', 'UnsqueezeV11',
'Squeeze', 'Unsqueeze', 'Reshape']
'Squeeze', 'Unsqueeze', 'Reshape', 'DepthToSpace']
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit, if you put down new code, would be good to alphabetize this list.

@etiotto etiotto linked an issue Oct 19, 2021 that may be closed by this pull request
@AlexandreEichenberger
Copy link
Collaborator

@etiotto Is the PR still needed? I see it having conflict, failure. Please close or update.

@etiotto
Copy link
Collaborator Author

etiotto commented Oct 27, 2021

This approach is no longer needed. The DepthToSpace operator has been implemented via the other PRs associated with work item #927. Closing.

@etiotto etiotto closed this Oct 27, 2021
@etiotto etiotto deleted the decompose_depth_to_space branch October 27, 2021 20:05
// Canonicalization for ONNXDepthToSpaceOp
// Rewrite:
// output = onnx.depthToSpace(x, bs, mode=[DCR|CRD])
// [B, C, H, W] = x.shape
Copy link
Collaborator

@chentong319 chentong319 Oct 28, 2021

Choose a reason for hiding this comment

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

What if x has known dim (-1)?
It seems to me that we can have two rules for different mode.
For mode==DCR, the rule can be (not precisely) written as following to handle non constant shape with just rule. Doable?

def DecomposeDepthToSpacePattern : Pat<`
((ONNXDepthToSpaceOp:$res $x, $bsAttr, $modeAttr),
  ((ONNXShape:$shape $x),
    (ONNXConcat:$newshape (ONNXClip $shape 0),
                                (ONNXConstant $bsAttr), 
                                (ONNXConstant $bsAttr), 
                                (ONNXDiv (ONNXClip $shape 1),  Constant($bsAttr*$bsAttr)),
                                (ONNXClip $shape 2),
                                (ONNXClip $shape 3)),
ONNXReshape $x $newShape)
[($modeAttr == "DCR")]>

Copy link
Collaborator

Choose a reason for hiding this comment

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

@etiotto should we reopen this PR to avoid lowering DepthToSpace to krnl?

@etiotto
Copy link
Collaborator Author

etiotto commented Nov 1, 2021

Currently we lower DepthToSpace in src/Conversions/ONNXToKernel/Tensor/DepthToSpace.cpp into ONNXReshape -> ONNXTranspose -> ONNXReshape. We did not need to create a new Krnl operation.

There are 2 cases to consider, when all dimensions of the shape for ONNX.Reshape are known at compile time or when they aren't. In the former we generate a onnx.Constant to encode the shape, in the later we have to generate code to compute the new shape.

I think the current approach is reasonable. It allows reuse of the code we already have to lower onnx.Reshape and onnx.Transpose so that, should we have a bug ior need to optimize codegen for those operations, we can leverage the effort. Also, the implementation of SpaceToDepth can leverage the same approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for DepthToSpace operator
4 participants