You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/12/19 12:34:05 UTC
[GitHub] [tvm] Alexey-Yazev opened a new pull request, #13645: [microNPU] Add relu6 relu_n1_to_1 test cases for Ethos-U
Alexey-Yazev opened a new pull request, #13645:
URL: https://github.com/apache/tvm/pull/13645
Tests are extended with cases with activations relu6 and relu_n1_to_1.
Does not fuse min and max operations with requantize if there are different scales as it is not supported on NPU.
cc @leandron, @ekalda, @lhutton1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ekalda commented on a diff in pull request #13645: [microNPU] Add relu6 relu_n1_to_1 test cases for Ethos-U
Posted by GitBox <gi...@apache.org>.
ekalda commented on code in PR #13645:
URL: https://github.com/apache/tvm/pull/13645#discussion_r1064848349
##########
python/tvm/relay/op/contrib/ethosu.py:
##########
@@ -938,12 +939,21 @@ def minimum_pattern() -> tvm.relay.dataflow_pattern.DFPattern:
"""
minimum = is_op("minimum")(wildcard(), wildcard())
optional_min_clip = is_op("clip")(minimum)
- optional_min_clip = is_op("qnn.requantize")(
- optional_min_clip, is_constant(), is_constant(), is_constant(), is_constant()
- )
return minimum | optional_min_clip
+def minimum_clip_requantize_pattern() -> tvm.relay.dataflow_pattern.DFPattern:
+ """
+ This function creates the pattern for minimum with fused RELU activation.
+ """
+ pattern = is_op("minimum")(wildcard(), wildcard())
+ pattern = is_op("clip")(pattern)
+ pattern = is_op("qnn.requantize")(
+ pattern, is_constant(), is_constant(), is_constant(), is_constant()
+ )
+ return pattern
Review Comment:
What's the motivation behind having two patterns instead of one with optional requantize if we use same params extractor (`MinParams`) with them?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ekalda merged pull request #13645: [microNPU] Add relu6 relu_n1_to_1 test cases for Ethos-U
Posted by GitBox <gi...@apache.org>.
ekalda merged PR #13645:
URL: https://github.com/apache/tvm/pull/13645
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Alexey-Yazev commented on a diff in pull request #13645: [microNPU] Add relu6 relu_n1_to_1 test cases for Ethos-U
Posted by GitBox <gi...@apache.org>.
Alexey-Yazev commented on code in PR #13645:
URL: https://github.com/apache/tvm/pull/13645#discussion_r1065533597
##########
python/tvm/relay/op/contrib/ethosu.py:
##########
@@ -938,12 +939,21 @@ def minimum_pattern() -> tvm.relay.dataflow_pattern.DFPattern:
"""
minimum = is_op("minimum")(wildcard(), wildcard())
optional_min_clip = is_op("clip")(minimum)
- optional_min_clip = is_op("qnn.requantize")(
- optional_min_clip, is_constant(), is_constant(), is_constant(), is_constant()
- )
return minimum | optional_min_clip
+def minimum_clip_requantize_pattern() -> tvm.relay.dataflow_pattern.DFPattern:
+ """
+ This function creates the pattern for minimum with fused RELU activation.
+ """
+ pattern = is_op("minimum")(wildcard(), wildcard())
+ pattern = is_op("clip")(pattern)
+ pattern = is_op("qnn.requantize")(
+ pattern, is_constant(), is_constant(), is_constant(), is_constant()
+ )
+ return pattern
Review Comment:
There are two patterns to support two cases:
a) when we can offload minimum + clip + qnn.requantize to NPU with one operation ethosu_binary_elementwise if there are same scales
b) when we can offload minimum + clip + qnn.requantize to NPU with two operations ethosu_binary_elementwise + ethosu_identity if there are different scales
Since there is a hardware limitation, we cannot perform min or max operation fused with requantize (please look at NPU_SET_OFM_SCALE https://developer.arm.com/documentation/102420/0200/Programmers-model/Command-stream/cmd1-commands-) when we have different scales.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Alexey-Yazev commented on pull request #13645: [microNPU] Add relu6 relu_n1_to_1 test cases for Ethos-U
Posted by GitBox <gi...@apache.org>.
Alexey-Yazev commented on PR #13645:
URL: https://github.com/apache/tvm/pull/13645#issuecomment-1378820135
> so I suspect that if the graph consists only RELU and nothing else, it doesn't get offloaded to NPU at all.
in this case requantize operation can be offloaded to NPU
> I think so far the approach has been to match all the variations of the operator and then output the right combination of Ethos-U ops, e.g. Resize2d. If there is a reason it can't be done that way, it would be good to document it there since it is not immediately obvious from the code why we need two patterns.
reason that two patterns are used is that we can understand whether this pattern is suitable after checking scales match that are performed in function is_valid
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tvm-bot commented on pull request #13645: [microNPU] Add relu6 relu_n1_to_1 test cases for Ethos-U
Posted by GitBox <gi...@apache.org>.
tvm-bot commented on PR #13645:
URL: https://github.com/apache/tvm/pull/13645#issuecomment-1357603443
<!---bot-comment-->
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
<!--bot-comment-ccs-start-->
* cc @Mousius, @leandron, @lhutton1 <sub>See [#10317](https://github.com/apache/tvm/issues/10317) for details</sub><!--bot-comment-ccs-end-->
<sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Alexey-Yazev commented on a diff in pull request #13645: [microNPU] Add relu6 relu_n1_to_1 test cases for Ethos-U
Posted by GitBox <gi...@apache.org>.
Alexey-Yazev commented on code in PR #13645:
URL: https://github.com/apache/tvm/pull/13645#discussion_r1059276969
##########
tests/python/contrib/test_ethosu/test_codegen.py:
##########
@@ -1109,6 +1109,45 @@ def leaky_relu_func(x):
)
+def test_tflite_relu6():
+ np.random.seed(0)
+ accel_type = "ethos-u55-128"
+ ifm_shape = (1, 12, 16, 8)
+
+ @tf.function
+ def relu6(x):
+ return tf.nn.relu6(x)
+
+ infra.compare_tvm_with_tflite(
+ relu6,
+ [ifm_shape],
+ accel_type,
+ enable_cascader=is_u55_accel_type(accel_type),
+ ranges=[(-1, 1)],
+ )
+
+
+def test_tflite_relu_n1_to_1():
+ np.random.seed(0)
+ accel_type = "ethos-u55-128"
+ ifm_shape = (1, 12, 16, 8)
+
+ @tf.function
+ def relu_n1_to_1(x):
+ """
+ The specific pattern will be replaced into RELU_N1_TO_1 by tflite.
+ """
+ return tf.math.maximum(-1.0, tf.math.minimum(x, 1.0))
Review Comment:
All relu operations are converted to clip operations in tflite frontend. It seems these tests are unnecessary perhaps it's worth adding a test for maximum operation when there are different scales and it's uploaded to the npu not by one operation as in other cases, but by two.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Alexey-Yazev commented on a diff in pull request #13645: [microNPU] Add relu6 relu_n1_to_1 test cases for Ethos-U
Posted by GitBox <gi...@apache.org>.
Alexey-Yazev commented on code in PR #13645:
URL: https://github.com/apache/tvm/pull/13645#discussion_r1067796699
##########
tests/python/contrib/test_ethosu/test_codegen.py:
##########
@@ -1109,6 +1109,77 @@ def leaky_relu_func(x):
)
+# conv2d + relu_n1_to_1 is used because separate activation is not offloaded to NPU.
+def test_tflite_relu_n1_to_1():
+ np.random.seed(0)
+ accel_type = "ethos-u55-256"
+ ifm_shape = (1, 55, 34, 3)
+ kernel_shape = (3, 2)
+ strides = (1, 1)
+ padding = (1, 0, 1, 1)
+
+ @tf.function
+ def conv2d_relu_n1_to_1(x):
+ tf_strides = [1, strides[0], strides[1], 1]
+ op = tf.pad(
+ x,
+ [[0, 0], [padding[0], padding[2]], [padding[1], padding[3]], [0, 0]],
+ "CONSTANT",
+ )
Review Comment:
There is no specific reason, it can be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ekalda commented on a diff in pull request #13645: [microNPU] Add relu6 relu_n1_to_1 test cases for Ethos-U
Posted by GitBox <gi...@apache.org>.
ekalda commented on code in PR #13645:
URL: https://github.com/apache/tvm/pull/13645#discussion_r1067227196
##########
tests/python/contrib/test_ethosu/test_codegen.py:
##########
@@ -1109,6 +1109,77 @@ def leaky_relu_func(x):
)
+# conv2d + relu_n1_to_1 is used because separate activation is not offloaded to NPU.
+def test_tflite_relu_n1_to_1():
+ np.random.seed(0)
+ accel_type = "ethos-u55-256"
+ ifm_shape = (1, 55, 34, 3)
+ kernel_shape = (3, 2)
+ strides = (1, 1)
+ padding = (1, 0, 1, 1)
+
+ @tf.function
+ def conv2d_relu_n1_to_1(x):
+ tf_strides = [1, strides[0], strides[1], 1]
+ op = tf.pad(
+ x,
+ [[0, 0], [padding[0], padding[2]], [padding[1], padding[3]], [0, 0]],
+ "CONSTANT",
+ )
Review Comment:
Why the preceding pad?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Alexey-Yazev commented on pull request #13645: [microNPU] Add relu6 relu_n1_to_1 test cases for Ethos-U
Posted by GitBox <gi...@apache.org>.
Alexey-Yazev commented on PR #13645:
URL: https://github.com/apache/tvm/pull/13645#issuecomment-1376970374
> (1) We should still check whether TFLite's RELU6 is correctly executed on NPU, some basic conv2d + RELU6 tests should do
in my opinion it will be enough to add separate tests for RELU6 and RELU_N1_TO_1 since RELU, RELU6, RELU_N1_TO_1
activations are converted to clip operation in the tflite frontend https://github.com/apache/tvm/blob/main/python/tvm/relay/frontend/tflite.py#L534 and other cases with basic operations will be covered by cases with relu activation or is it necessary to add tests for RELU6, RELU_N1_TO_1 with other operations?
> (2) I'm confused about the min/max changes - is it an unrelated bugfix or necessary for RELU_N1_TO_1 implementation?
this is adding restrictions according to the hardware
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ekalda commented on pull request #13645: [microNPU] Add relu6 relu_n1_to_1 test cases for Ethos-U
Posted by GitBox <gi...@apache.org>.
ekalda commented on PR #13645:
URL: https://github.com/apache/tvm/pull/13645#issuecomment-1377502376
> > (1) We should still check whether TFLite's RELU6 is correctly executed on NPU, some basic conv2d + RELU6 tests should do
>
> in my opinion it will be enough to add separate tests for RELU6 and RELU_N1_TO_1 since RELU, RELU6, RELU_N1_TO_1 activations are converted to clip operation in the tflite frontend https://github.com/apache/tvm/blob/main/python/tvm/relay/frontend/tflite.py#L534 and other cases with basic operations will be covered by cases with relu activation or is it necessary to add tests for RELU6, RELU_N1_TO_1 with other operations?
>
We don't have a pattern matcher for a solitary Relay CLIP operation, so I suspect that if the graph consists only RELU and nothing else, it doesn't get offloaded to NPU at all. That's why I was suggesting it is tested in conjunction with another operator that matches an optional RELU, but I agree that there is no need to test it with all of them. But since both, RELU6 and RELU_N1_TO_1, are distinct TFLite operators, it would be good to have some testing for them to claim that they are supported.
> > (2) I'm confused about the min/max changes - is it an unrelated bugfix or necessary for RELU_N1_TO_1 implementation?
>
> this is adding restrictions according to the hardware
To me this looks like fixing what was an oversight in the initial implementation of binary elementwise and is not related to adding support to two new activation functions (which is what the title of this PR claims) and in general should be a separate PR, but I don't think it worth the trouble to split up this PR since it is quite small.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ekalda commented on a diff in pull request #13645: [microNPU] Add relu6 relu_n1_to_1 test cases for Ethos-U
Posted by GitBox <gi...@apache.org>.
ekalda commented on code in PR #13645:
URL: https://github.com/apache/tvm/pull/13645#discussion_r1065956144
##########
python/tvm/relay/op/contrib/ethosu.py:
##########
@@ -938,12 +939,21 @@ def minimum_pattern() -> tvm.relay.dataflow_pattern.DFPattern:
"""
minimum = is_op("minimum")(wildcard(), wildcard())
optional_min_clip = is_op("clip")(minimum)
- optional_min_clip = is_op("qnn.requantize")(
- optional_min_clip, is_constant(), is_constant(), is_constant(), is_constant()
- )
return minimum | optional_min_clip
+def minimum_clip_requantize_pattern() -> tvm.relay.dataflow_pattern.DFPattern:
+ """
+ This function creates the pattern for minimum with fused RELU activation.
+ """
+ pattern = is_op("minimum")(wildcard(), wildcard())
+ pattern = is_op("clip")(pattern)
+ pattern = is_op("qnn.requantize")(
+ pattern, is_constant(), is_constant(), is_constant(), is_constant()
+ )
+ return pattern
Review Comment:
Ok, I see how that works now :) I think so far the approach has been to match all the variations of the operator and then output the right combination of Ethos-U ops, e.g. `Resize2d`. If there is a reason it can't be done that way, it would be good to document it there since it is not immediately obvious from the code why we need two patterns.
I think it also needs some legalization tests to check that:
(1) min/max with matching scales -> `ethosu_binary_elementwise`
(2) min/max with different scales -> `ethosu_binary_elementwise` + `ethosu_identity`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ekalda commented on a diff in pull request #13645: [microNPU] Add relu6 relu_n1_to_1 test cases for Ethos-U
Posted by GitBox <gi...@apache.org>.
ekalda commented on code in PR #13645:
URL: https://github.com/apache/tvm/pull/13645#discussion_r1058940323
##########
tests/python/contrib/test_ethosu/test_codegen.py:
##########
@@ -1109,6 +1109,45 @@ def leaky_relu_func(x):
)
+def test_tflite_relu6():
+ np.random.seed(0)
+ accel_type = "ethos-u55-128"
+ ifm_shape = (1, 12, 16, 8)
+
+ @tf.function
+ def relu6(x):
+ return tf.nn.relu6(x)
+
+ infra.compare_tvm_with_tflite(
+ relu6,
+ [ifm_shape],
+ accel_type,
+ enable_cascader=is_u55_accel_type(accel_type),
+ ranges=[(-1, 1)],
+ )
+
+
+def test_tflite_relu_n1_to_1():
+ np.random.seed(0)
+ accel_type = "ethos-u55-128"
+ ifm_shape = (1, 12, 16, 8)
+
+ @tf.function
+ def relu_n1_to_1(x):
+ """
+ The specific pattern will be replaced into RELU_N1_TO_1 by tflite.
+ """
+ return tf.math.maximum(-1.0, tf.math.minimum(x, 1.0))
Review Comment:
Out of interest, what Relay does this get lowered to? I always thought all the different kinds of RELUs were lowered to the same Relay clip with just different min and max and then this clip was pattern matched as a part of all the different op pattern matchers and added to the primitive op's `clip_min` and `clip_max`, but as we don't pattern match for standalone clip and this test here passes, I suppose it gets lowered to something different?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org