Skip to content

Conversation

@lixiaoquan
Copy link
Contributor

With this PR, it is easy to trace a relay CallNode to its source in an imported model. It looks like:

%57 = nn.relu(%56) /* resnet_model/Relu_9 */;
%58 = transpose(%resnet_model/conv2d_12/kernel, axes=[3, 2, 0, 1]);
%59 = nn.conv2d(%57, %58, padding=[0, 0, 0, 0], channels=128, kernel_size=[1, 1]) /* resnet_model/conv2d_12/Conv2D */;
%60 = nn.batch_norm(%59, %resnet_model/batch_normalization_10/gamma, %resnet_model/batch_normalization_10/beta, %resnet_model/batch_normalization_10/moving_mean, %resnet_model/batch_normalization_10/moving_variance, epsilon=1.001e-05f) /* resnet_model/batch_normalization_10/FusedBatchNorm */;

I'm not sure whether it is the correct to use span.SourceName to store node name, but line, column doesn't make sense for an imported model.

@jroesch @junrushao1994 @zhiics Could you please help to review? Thanks

@FrozenGene
Copy link
Member

Ah...I think it is a nice feature and should enable it in all frontend, this is good for profiling (like we use debug graph runtime to see every layer's output). But I remember @tqchen said we will have another WIP feature to do this?

@tqchen
Copy link
Member

tqchen commented Nov 9, 2020

I now think that span is the right way to do it, as long as we don't rely the information in passes.

Copy link
Member

@zhiics zhiics left a comment

Choose a reason for hiding this comment

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

LGTM. I think this is also what @jroesch was adding.

@tqchen
Copy link
Member

tqchen commented Nov 9, 2020

Perhaps we could also do similar things for other frontends

@tqchen tqchen merged commit eb1fa29 into apache:main Nov 9, 2020
@jroesch
Copy link
Member

jroesch commented Nov 11, 2020

@lixiaoquan I think we can do this, but I am not sure if overloading the source spans are the right way? perhaps we can sub-class or modify spans for handling whether they point into a source file or into an imported graph? I think it might be good to clarify this design.

@lixiaoquan
Copy link
Contributor Author

@jroesch I agree with you. It looks like a makeshift to use SourceName to store node name, I did this because it introduced very little change. I think we can add a hint field. I added a thread https://discuss.tvm.apache.org/t/expand-span-for-imported-module/8435

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Dec 2, 2020
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Dec 4, 2020
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants