Support ingesting Opset 15 Shape Op #1684

philass · 2022-09-08T04:20:22Z

This PR adds support for shape inference for Opset-15 shape op.

Opset 15 added a default start attribute to the shape op (defaults to 0), and a optional end attribute.

These changes are not breaking changes to the older Shape op. Therefor we do not need to create a new ShapeOp to support the difference in API.

Signed-off-by: Philip Lassen <[email protected]>

AlexandreEichenberger · 2022-09-12T14:42:54Z

Do you mind adding a short description of the transfer, tx

Signed-off-by: Philip Lassen <[email protected]>

AlexandreEichenberger · 2022-09-13T15:26:08Z

src/Accelerators/NNPA/Conversion/ONNXToZHigh/RewriteONNXForZHigh.td

@@ -88,6 +88,17 @@ def replaceONNXBatchNormalizationInferenceModePattern : Pattern<
 // 
 //===----------------------------------------------------------------------===//

+// Get a type for a tensor that stores the shape of another tensor.
+def GetStart: NativeCodeCall<
+  //"IntegerType::get($_builder, 64, IntegerType::Signed)"


Should remove commented code. Have you though about adding a custom builder of the operation that does not have the start/end and would automatically generate them.... instead of modifying each of the patterns involved with the new shape op?

I did consider adding a custom builder. In all honesty I tried and was having some issues getting it to work.

But you makes good points. I have added a Native code call to more succinctly create ONNX Shape ops

// Create an ONNX Shape Op with type def CreateShapeOp: NativeCodeCall< "$_builder.create<mlir::ONNXShapeOp>($_loc, $0, $1, IntegerAttr(), 0)" >;

And I now call this, to avoid having to use the start and end attributes everywhere.

Awesome, glad you found a way to simplify the code

AlexandreEichenberger · 2022-09-13T15:30:33Z

src/Dialect/ONNX/ONNXOpsHelper.cpp

+  if (end < 0) {
+    end = end + rank;
+  }
+


would it make sense to add asserts to make sure that start and end are now in inclusively 0..rank-1 for start, 0..rank for end?

Good catch! This was actually wrong because the behaviour is meant to do be the following

"Optional attributes start and end can be used to compute a slice of the input tensor's shape."
"If start axis is omitted, the slice starts from axis 0."
"The end axis, if specified, is exclusive (and the returned value will not include the size of that axis)."
"If the end axis is omitted, the axes upto the last one will be included."
"Negative axes indicate counting back from the last axis."
"Note that axes will be clipped to the range [0, r-1], where r is the"
"rank of the input tensor if they are out-of-range (after adding r in the case of"
"negative axis). Thus, specifying any end value > r is equivalent to specifying an end"
"value of r, and specifying any start value < -r is equivalent to specifying a start"
"value of 0."

I've rejigged the logic to use getDataShapeBounds which handles the clipping logic. This puts the normalizing behavior in one place, where it is handled correctly

Signed-off-by: Philip Lassen <[email protected]>

AlexandreEichenberger

getting very close, thanks for the improvement. Left a question.

AlexandreEichenberger · 2022-09-15T18:16:06Z

src/Dialect/ONNX/Rewrite.cpp

+  int64_t rank = inType.getRank();
+
+  int64_t endValue = end.has_value() ? end.value() : rank;
+


would it make sense to have an assert here for sizes, or better, it could be added in the verifier (apologies if it is already there)

AlexandreEichenberger · 2022-09-15T18:19:16Z

src/Dialect/ONNX/ShapeInference/Shape.cpp

+// of negative axis). Thus, specifying any end value > r is equivalent to
+// specifying an end value of r, and specifying any start value < -r is
+// equivalent to specifying a start value of 0."
+int64_t normalize(int64_t axis, int64_t rank) {


I see what you do here, it sounds reasonable but is it the standard? Or should we instead report an error?
In general, we have erred in favor of reporting errors so as to make the user aware of the issue, instead of "fixing" the model the way we "hope" the user wanted.
As a minimum, I would issue a warning.
Note also that if I recall correctly, sometime the clipping is at rank and sometime rank-1... so this method may not be applied everywhere.

@chentong319 do you have an opinion on this issue?

Sorry if this was confusing. This was actually lifted for the ONNX specification

Shape-15
Takes a tensor as input and outputs an 1D int64 tensor containing the shape of the input tensor. Optional attributes start and end can be used to compute a slice of the input tensor's shape. If start axis is omitted, the slice starts from axis 0. The end axis, if specified, is exclusive (and the returned value will not include the size of that axis). If the end axis is omitted, the axes upto the last one will be included. Negative axes indicate counting back from the last axis. Note that axes will be clamped to the range [0, r-1], where r is the rank of the input tensor if they are out-of-range (after adding r in the case of negative axis). Thus, specifying any end value > r is equivalent to specifying an end value of r, and specifying any start value < -r is equivalent to specifying a start value of 0.

It seems weird, but I have based it off the ONNX spec of Shape.

Great, I would then just reflect this policy in the function name, so that others may not be tempted to use it when it is not applicable. Add something of the sort "ClampedPerSpec" or something similar.

AlexandreEichenberger

LGTM

AlexandreEichenberger · 2022-09-15T18:46:01Z

@philass I believe you should now be able to squash PRs that have been approved.

philass · 2022-09-15T18:49:56Z

@AlexandreEichenberger It looks like I can!

Thanks for the review as always! I appreciate you taking the time :)

AlexandreEichenberger · 2022-09-16T14:50:54Z

@philass is it ready to go?

AlexandreEichenberger · 2022-09-16T14:57:32Z

@philass

there seems to be some compilation error linked with the NNPA. Do you mind looking into it, and if it is something that we need to do, let us know

[91m/workdir/onnx-mlir/src/Accelerators/NNPA/Conversion/ONNXToZHigh/RewriteONNXForZHigh.td:249:1: error: op 'onnx.Shape' argument number mismatch: 2 in pattern vs. 3 in definition
def rewriteSoftmaxNDto2D: Pat<
^
�[0m�[91mmake[2]: *** [src/Accelerators/NNPA/Conversion/ONNXToZHigh/CMakeFiles/OMONNXRewriteONNXForZHighIncGen.dir/build.make:106: src/Accelerators/NNPA/Conversion/ONNXToZHigh/ONNXRewriteONNXForZHigh.inc] Error 1
�[0m�[91mmake[1]: *** [CMakeFiles/Makefile2:10481: src/Accelerators/NNPA/Conversion/ONNXToZHigh/CMakeFiles/OMONNXRewriteONNXForZHighIncGen.dir/all] Error 2

philass · 2022-09-16T21:59:05Z

@philass

there seems to be some compilation error linked with the NNPA. Do you mind looking into it, and if it is something that we need to do, let us know

[91m/workdir/onnx-mlir/src/Accelerators/NNPA/Conversion/ONNXToZHigh/RewriteONNXForZHigh.td:249:1: error: op 'onnx.Shape' argument number mismatch: 2 in pattern vs. 3 in definition
def rewriteSoftmaxNDto2D: Pat<
^
�[0m�[91mmake[2]: *** [src/Accelerators/NNPA/Conversion/ONNXToZHigh/CMakeFiles/OMONNXRewriteONNXForZHighIncGen.dir/build.make:106: src/Accelerators/NNPA/Conversion/ONNXToZHigh/ONNXRewriteONNXForZHigh.inc] Error 1
�[0m�[91mmake[1]: *** [CMakeFiles/Makefile2:10481: src/Accelerators/NNPA/Conversion/ONNXToZHigh/CMakeFiles/OMONNXRewriteONNXForZHighIncGen.dir/all] Error 2

Yeah I'll look into this. I won't merge and squash until this is resolved. I'll have some free time this weekend to look into it

Signed-off-by: Philip Lassen <[email protected]>

jenkins-droid · 2022-09-17T03:34:43Z

Jenkins Linux s390x Build #7632 [push] Support ingesting Opset ... started at 23:34

jenkins-droid · 2022-09-17T03:34:45Z

Jenkins Linux amd64 Build #7616 [push] Support ingesting Opset ... started at 22:34

jenkins-droid · 2022-09-17T03:34:47Z

Jenkins Linux ppc64le Build #6681 [push] Support ingesting Opset ... started at 23:36

jenkins-droid · 2022-09-17T04:35:18Z

Jenkins Linux amd64 Build #7616 [push] Support ingesting Opset ... passed after 1 hr 0 min

jenkins-droid · 2022-09-17T05:00:30Z

Jenkins Linux s390x Build #7632 [push] Support ingesting Opset ... passed after 1 hr 25 min

jenkins-droid · 2022-09-17T05:08:35Z

Jenkins Linux ppc64le Build #6681 [push] Support ingesting Opset ... passed after 1 hr 33 min

* Shape op replace Signed-off-by: Philip Lassen <[email protected]> * Run clang format Signed-off-by: Philip Lassen <[email protected]> * Fix NNPA Shape for opset 15 Signed-off-by: Philip Lassen <[email protected]> * Fix shape in NNPA Signed-off-by: Philip Lassen <[email protected]> * Some more progress Signed-off-by: Philip Lassen <[email protected]> * Fix shape inference and lowering Signed-off-by: Philip Lassen <[email protected]> * Use std::tie Signed-off-by: Philip Lassen <[email protected]> * Add clean up Signed-off-by: Philip Lassen <[email protected]> * Rename ONNXShape to CreateShapeOp Signed-off-by: Philip Lassen <[email protected]> * fix lit tests Signed-off-by: Philip Lassen <[email protected]> * Fix tablegen pattern and improve normalize function name Signed-off-by: Philip Lassen <[email protected]> Signed-off-by: Philip Lassen <[email protected]> Co-authored-by: Alexandre Eichenberger <[email protected]> Signed-off-by: Stella Stamenova <[email protected]>

Shape op replace

46729cb

Signed-off-by: Philip Lassen <[email protected]>

philass force-pushed the plassen/opset-15-shape-op branch from bf2e006 to 46729cb Compare September 8, 2022 04:21

philass added 4 commits September 7, 2022 21:23

Run clang format

f125c6b

Signed-off-by: Philip Lassen <[email protected]>

Fix NNPA Shape for opset 15

7b16406

Signed-off-by: Philip Lassen <[email protected]>

Fix shape in NNPA

4e24738

Signed-off-by: Philip Lassen <[email protected]>

Some more progress

ae9726a

Signed-off-by: Philip Lassen <[email protected]>

philass added 2 commits September 12, 2022 21:34

Fix shape inference and lowering

e75b833

Signed-off-by: Philip Lassen <[email protected]>

Use std::tie

39865f5

Signed-off-by: Philip Lassen <[email protected]>

philass changed the title ~~Shape op replace~~ Ingest Opset15 - Shape op Sep 13, 2022

philass changed the title ~~Ingest Opset15 - Shape op~~ Support Opset 15 Shape Op Sep 13, 2022

philass changed the title ~~Support Opset 15 Shape Op~~ Support ingesting Opset 15 Shape Op Sep 13, 2022

philass marked this pull request as ready for review September 13, 2022 08:25

AlexandreEichenberger reviewed Sep 13, 2022

View reviewed changes

philass added 3 commits September 14, 2022 13:51

Add clean up

0716df1

Signed-off-by: Philip Lassen <[email protected]>

Rename ONNXShape to CreateShapeOp

a5d7b24

Signed-off-by: Philip Lassen <[email protected]>

fix lit tests

9209217

Signed-off-by: Philip Lassen <[email protected]>

AlexandreEichenberger reviewed Sep 15, 2022

View reviewed changes

AlexandreEichenberger approved these changes Sep 15, 2022

View reviewed changes

Merge branch 'main' into plassen/opset-15-shape-op

7fc0824

AlexandreEichenberger mentioned this pull request Sep 16, 2022

Simplify shape-related operations #1695

Merged

tungld mentioned this pull request Sep 17, 2022

Add decompositions for v11 split, squeeze, and unsqueeze #1702

Merged

Fix tablegen pattern and improve normalize function name

e3f773f

Signed-off-by: Philip Lassen <[email protected]>

philass merged commit ecc8404 into onnx:main Sep 17, 2022

philass deleted the plassen/opset-15-shape-op branch September 17, 2022 03:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ingesting Opset 15 Shape Op #1684

Support ingesting Opset 15 Shape Op #1684

philass commented Sep 8, 2022 •

edited

Loading

AlexandreEichenberger commented Sep 12, 2022

AlexandreEichenberger Sep 13, 2022

philass Sep 14, 2022

AlexandreEichenberger Sep 15, 2022

AlexandreEichenberger Sep 13, 2022

philass Sep 14, 2022

AlexandreEichenberger left a comment

AlexandreEichenberger Sep 15, 2022

AlexandreEichenberger Sep 15, 2022

philass Sep 15, 2022

AlexandreEichenberger Sep 15, 2022

AlexandreEichenberger left a comment

AlexandreEichenberger commented Sep 15, 2022 •

edited

Loading

philass commented Sep 15, 2022

AlexandreEichenberger commented Sep 16, 2022

AlexandreEichenberger commented Sep 16, 2022

philass commented Sep 16, 2022

jenkins-droid commented Sep 17, 2022

jenkins-droid commented Sep 17, 2022

jenkins-droid commented Sep 17, 2022

jenkins-droid commented Sep 17, 2022

jenkins-droid commented Sep 17, 2022

jenkins-droid commented Sep 17, 2022

		int64_t rank = inType.getRank();

		int64_t endValue = end.has_value() ? end.value() : rank;

Support ingesting Opset 15 Shape Op #1684

Support ingesting Opset 15 Shape Op #1684

Conversation

philass commented Sep 8, 2022 • edited Loading

AlexandreEichenberger commented Sep 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandreEichenberger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandreEichenberger left a comment

Choose a reason for hiding this comment

AlexandreEichenberger commented Sep 15, 2022 • edited Loading

philass commented Sep 15, 2022

AlexandreEichenberger commented Sep 16, 2022

AlexandreEichenberger commented Sep 16, 2022

philass commented Sep 16, 2022

jenkins-droid commented Sep 17, 2022

jenkins-droid commented Sep 17, 2022

jenkins-droid commented Sep 17, 2022

jenkins-droid commented Sep 17, 2022

jenkins-droid commented Sep 17, 2022

jenkins-droid commented Sep 17, 2022

philass commented Sep 8, 2022 •

edited

Loading

AlexandreEichenberger commented Sep 15, 2022 •

edited

Loading