[TF frontend][bugfix]Avoid making a new node when already has span info

[xqdan]

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 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?

[tqchen]

cc @zhiics can you please help manage this PR Thank you

[zhiics]

could you add a test case for these two lines?

[xqdan]

I'd love to, but I don't know how to create unitest for pb parsering flow, any suggestion? thanks

[kevinthesun]

Does this require pb parsing or directly creating a tf program would be sufficient?

[xqdan]

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.

[kevinthesun]

I think we can refer to the existing unit/integration tests for tf frontend. Most of them just create a tf graph.

[lixiaoquan]

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.

tvm/include/tvm/ir/transform.h

Line 88 in 8131364

int opt_level{2};

[xqdan]

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.

tvm/include/tvm/ir/transform.h

Line 88 in 8131364

int opt_level{2};

Yes，EliminateCommonSubexpr() can remove redundant operations, however better to fix at the very beginning.

[xqdan]

@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.

[zhiics]

@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.

[zhiics]

Thanks @xqdan @kevinthesun