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 2021/04/06 09:56:26 UTC
[GitHub] [tvm] xqdan opened a new pull request #7789: [TF frontend][bugfix]Avoid making a new node when already has span info
xqdan opened a new pull request #7789:
URL: https://github.com/apache/tvm/pull/7789
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
@lixiaoquan @zhiics @srkreddy1238
This bug will lead redundent operations in graph ir when parsering bert:
```
%4 = add(%0, %3) /* bert/embeddings/add */;
%5 = strided_slice(meta[relay.Constant][1], begin=[0, 0], end=[128, -1], strides=[1, 1], slice_mode="size") /* bert/embeddings/Slice */;
%6 = reshape(%5, newshape=[1, 128, 768]) /* bert/embeddings/Reshape_4 */;
%7 = add(%4, %6) /* bert/embeddings/add_1 */;
%8 = mean(%7, axis=[2], keepdims=True) /* bert/embeddings/LayerNorm/moments/StopGradient */;
%9 = subtract(%7, %8);
%10 = multiply(%9, %9) /* bert/embeddings/LayerNorm/moments/SquaredDifference */;
%11 = mean(%10, axis=[2], keepdims=True) /* bert/embeddings/LayerNorm/moments/variance */;
%12 = add(%11, 1e-12f) /* bert/embeddings/LayerNorm/batchnorm/add */;
%13 = power(%12, -0.5f) /* bert/embeddings/LayerNorm/batchnorm/Rsqrt */;
%14 = multiply(%13, meta[relay.Constant][2]) /* bert/embeddings/LayerNorm/batchnorm/mul */;
%15 = multiply(%7, %14) /* bert/embeddings/LayerNorm/batchnorm/mul_1 */;
%16 = mean(%7, axis=[2], keepdims=True) /* bert/embeddings/LayerNorm/moments/mean */; --> %16 is the same as %8
%17 = multiply(%16, %14) /* bert/embeddings/LayerNorm/batchnorm/mul_2 */;
%18 = subtract(meta[relay.Constant][3], %17) /* bert/embeddings/LayerNorm/batchnorm/sub */;
```
The last mean is redundent op created by _set_span.
Since this is pb parsering flow, does anyone know how to make a unitest?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] xqdan edited a comment on pull request #7789: [TF frontend][bugfix]Avoid making a new node when already has span info
Posted by GitBox <gi...@apache.org>.
xqdan edited a comment on pull request #7789:
URL: https://github.com/apache/tvm/pull/7789#issuecomment-813992258
> It is ok to me.
>
> I guess this is caused by different nodes in PB are being parsed to the same Relay node, so they have different names.
>
> I think EliminateCommonSubexpr() should be able to eliminate the redundancy, but it requires opt_level 3, so it is disabled by default.
>
> https://github.com/apache/tvm/blob/813136401a11a49d6c15e6013c34dd822a5c4ff6/include/tvm/ir/transform.h#L88
Yes，EliminateCommonSubexpr() can remove redundant operations, however better to fix at the very beginning.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zhiics commented on pull request #7789: [TF frontend][bugfix]Avoid making a new node when already has span info
Posted by GitBox <gi...@apache.org>.
zhiics commented on pull request #7789:
URL: https://github.com/apache/tvm/pull/7789#issuecomment-815470787
@xqdan thanks for adding the test. But I think we probably don't need to create a new test file though. Instead, we can put it under test_forward as the operator tests are sitting there.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] lixiaoquan edited a comment on pull request #7789: [TF frontend][bugfix]Avoid making a new node when already has span info
Posted by GitBox <gi...@apache.org>.
lixiaoquan edited a comment on pull request #7789:
URL: https://github.com/apache/tvm/pull/7789#issuecomment-813926050
It is ok to me.
I guess this is caused by different nodes in PB are being parsed to the same Relay node, so they have different names.
I think EliminateCommonSubexpr() should be able to eliminate the redundancy, but it requires opt_level 3, so it is disabled by default.
https://github.com/apache/tvm/blob/813136401a11a49d6c15e6013c34dd822a5c4ff6/include/tvm/ir/transform.h#L88
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] lixiaoquan commented on pull request #7789: [TF frontend][bugfix]Avoid making a new node when already has span info
Posted by GitBox <gi...@apache.org>.
lixiaoquan commented on pull request #7789:
URL: https://github.com/apache/tvm/pull/7789#issuecomment-813926050
I guess this is caused by different nodes in PB are being parsed to the same Relay node, so they have different names.
I think EliminateCommonSubexpr() should be able to eliminate the redundancy, but it requires opt_level 3, so it is disabled by default.
https://github.com/apache/tvm/blob/813136401a11a49d6c15e6013c34dd822a5c4ff6/include/tvm/ir/transform.h#L88
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] xqdan commented on pull request #7789: [TF frontend][bugfix]Avoid making a new node when already has span info
Posted by GitBox <gi...@apache.org>.
xqdan commented on pull request #7789:
URL: https://github.com/apache/tvm/pull/7789#issuecomment-813992258
> It is ok to me.
>
> I guess this is caused by different nodes in PB are being parsed to the same Relay node, so they have different names.
>
> I think EliminateCommonSubexpr() should be able to eliminate the redundancy, but it requires opt_level 3, so it is disabled by default.
>
> https://github.com/apache/tvm/blob/813136401a11a49d6c15e6013c34dd822a5c4ff6/include/tvm/ir/transform.h#L88
Yes，EliminateCommonSubexpr() can
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kevinthesun commented on a change in pull request #7789: [TF frontend][bugfix]Avoid making a new node when already has span info
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #7789:
URL: https://github.com/apache/tvm/pull/7789#discussion_r607416994
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -3851,11 +3851,11 @@ def _convert_operator(
@staticmethod
def _set_span(sym, node_name):
span = tvm.relay.Span(tvm.relay.SourceName(node_name), 0, 0, 0, 0)
- if isinstance(sym, _expr.Call):
+ if isinstance(sym, _expr.Call) and sym.span is None:
Review comment:
Does this require pb parsing or directly creating a tf program would be sufficient?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zhiics commented on a change in pull request #7789: [TF frontend][bugfix]Avoid making a new node when already has span info
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #7789:
URL: https://github.com/apache/tvm/pull/7789#discussion_r609335683
##########
File path: tests/python/frontend/tensorflow/test_forward.py
##########
@@ -5451,5 +5451,44 @@ def test_forward_unique_with_counts():
_test_unique_with_counts(20, dtype, is_dyn)
+#######################################################################
+# check graph ir for nn.moments
+# ------------
+
+SEMVER = '#[version = "0.0.5"]\n'
+
+
+def run_from_tensorflow(graph):
Review comment:
This method is not quite useful though since you only use it once and it only has two lines
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] xqdan commented on a change in pull request #7789: [TF frontend][bugfix]Avoid making a new node when already has span info
Posted by GitBox <gi...@apache.org>.
xqdan commented on a change in pull request #7789:
URL: https://github.com/apache/tvm/pull/7789#discussion_r607426735
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -3851,11 +3851,11 @@ def _convert_operator(
@staticmethod
def _set_span(sym, node_name):
span = tvm.relay.Span(tvm.relay.SourceName(node_name), 0, 0, 0, 0)
- if isinstance(sym, _expr.Call):
+ if isinstance(sym, _expr.Call) and sym.span is None:
Review comment:
> Does this require pb parsing or directly creating a tf program would be sufficient?
You are right，where can I find an exmple? I know the basic idea is creatig a tf graph, using from_tensorflow to handle it, then check the output graph ir.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] xqdan commented on pull request #7789: [TF frontend][bugfix]Avoid making a new node when already has span info
Posted by GitBox <gi...@apache.org>.
xqdan commented on pull request #7789:
URL: https://github.com/apache/tvm/pull/7789#issuecomment-814565350
@zhiics @kevinthesun Added a unites, please review. BTW it's easy to add nn.moments as unitest because we met it, however I don't know which tf graph/layer can tigger tuple branch, appreciate if you have suggestions.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zhiics commented on a change in pull request #7789: [TF frontend][bugfix]Avoid making a new node when already has span info
Posted by GitBox <gi...@apache.org>.
zhiics commented on a change in pull request #7789:
URL: https://github.com/apache/tvm/pull/7789#discussion_r606950774
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -3851,11 +3851,11 @@ def _convert_operator(
@staticmethod
def _set_span(sym, node_name):
span = tvm.relay.Span(tvm.relay.SourceName(node_name), 0, 0, 0, 0)
- if isinstance(sym, _expr.Call):
+ if isinstance(sym, _expr.Call) and sym.span is None:
Review comment:
could you add a test case for these two lines?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] kevinthesun commented on a change in pull request #7789: [TF frontend][bugfix]Avoid making a new node when already has span info
Posted by GitBox <gi...@apache.org>.
kevinthesun commented on a change in pull request #7789:
URL: https://github.com/apache/tvm/pull/7789#discussion_r608253193
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -3851,11 +3851,11 @@ def _convert_operator(
@staticmethod
def _set_span(sym, node_name):
span = tvm.relay.Span(tvm.relay.SourceName(node_name), 0, 0, 0, 0)
- if isinstance(sym, _expr.Call):
+ if isinstance(sym, _expr.Call) and sym.span is None:
Review comment:
I think we can refer to the existing unit/integration tests for tf frontend. Most of them just create a tf graph.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zhiics merged pull request #7789: [TF frontend][bugfix]Avoid making a new node when already has span info
Posted by GitBox <gi...@apache.org>.
zhiics merged pull request #7789:
URL: https://github.com/apache/tvm/pull/7789
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] xqdan closed pull request #7789: [TF frontend][bugfix]Avoid making a new node when already has span info
Posted by GitBox <gi...@apache.org>.
xqdan closed pull request #7789:
URL: https://github.com/apache/tvm/pull/7789
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] zhiics commented on pull request #7789: [TF frontend][bugfix]Avoid making a new node when already has span info
Posted by GitBox <gi...@apache.org>.
zhiics commented on pull request #7789:
URL: https://github.com/apache/tvm/pull/7789#issuecomment-816359060
Thanks @xqdan @kevinthesun
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] xqdan commented on a change in pull request #7789: [TF frontend][bugfix]Avoid making a new node when already has span info
Posted by GitBox <gi...@apache.org>.
xqdan commented on a change in pull request #7789:
URL: https://github.com/apache/tvm/pull/7789#discussion_r607075906
##########
File path: python/tvm/relay/frontend/tensorflow.py
##########
@@ -3851,11 +3851,11 @@ def _convert_operator(
@staticmethod
def _set_span(sym, node_name):
span = tvm.relay.Span(tvm.relay.SourceName(node_name), 0, 0, 0, 0)
- if isinstance(sym, _expr.Call):
+ if isinstance(sym, _expr.Call) and sym.span is None:
Review comment:
I'd love to, but I don't know how to create unitest for pb parsering flow, any suggestion? thanks
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #7789: [TF frontend][bugfix]Avoid making a new node when already has span info
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7789:
URL: https://github.com/apache/tvm/pull/7789#issuecomment-812858365
cc @zhiics can you please help manage this PR Thank you
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org