Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[tvm][any] broadcast with values other than one #3967

Merged
merged 4 commits into from
Oct 11, 2019
Merged

Conversation

zhiics
Copy link
Member

@zhiics zhiics commented Sep 18, 2019

This PR makes relay Any type broadcast with values other than one, i.e.:

x = relay.var('x', shape=(3, 2), dtype="float32")
y = relay.var('y', shape=(relay.Any(), 2), dtype="float32")
relay.add(x, y)

It makes all symbolic vars as auto_broadcast if they haven't been bound to compact buffer before.

@icemelon9 @yzhliu @tqchen @jroesch

@jroesch
Copy link
Member

jroesch commented Sep 18, 2019

I am a bit confused by the semantics, how does this work? for example imagine that I have two any dimensions which will be (say 20, and 15 at runtime) it doesn't make sense to broad cast against non-1 dimensions.

@zhiics
Copy link
Member Author

zhiics commented Sep 18, 2019

@jroesch sorry. I think the example I gave missed something. I updated it.

@zhiics
Copy link
Member Author

zhiics commented Sep 18, 2019

close for now. need more consideration about the auto_broadcast binding.

@zhiics zhiics closed this Sep 18, 2019
@zhiics zhiics reopened this Sep 18, 2019
@junrushao
Copy link
Member

Hey this is super helpful!

Btw, I notice that in the compile engine it assumes that checked type exists. Does it mean that for now we are only able to deal with the case that dims are fixed and certain dimensions are Any?

@zhiics
Copy link
Member Author

zhiics commented Sep 20, 2019

@junrushao1994 Yes, you are right, only the bound is dynamic but rank is fixed.

@kevinthesun
Copy link
Contributor

It will generate runtime error when actual shape is incompatible?

@zhiics
Copy link
Member Author

zhiics commented Sep 20, 2019

@kevinthesun Yes, both the interpreter and VM would fail if the real values are incompatible. I can probably add a few tests for them as well.

@icemelon
Copy link
Member

icemelon commented Sep 20, 2019

It will generate runtime error when actual shape is incompatible?

The shape function will perform the type checking at runtime (see https://github.com/dmlc/tvm/blob/master/python/tvm/relay/op/_tensor.py#L130).
However, I think there's no type checking in the TOPI generated kernels.

@zhiics
Copy link
Member Author

zhiics commented Sep 20, 2019

@icemelon9 yeah, that might be a different between Any and tvm symbolic var.

Copy link
Member

@yzhliu yzhliu left a comment

Choose a reason for hiding this comment

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

Is it possible to have a pass replace attr::buffer_bind_scope ? current impl seems to be a little bit adhoc.

@zhiics
Copy link
Member Author

zhiics commented Sep 23, 2019

@yzhliu Yeah, I actually thought about replacing it as well. But I ended up skipping it because it looks there is not much difference. I think we probably want to keep compact memory as much as possible because auto broadcast will bring in the if_then_else guard which may not as performant as the compact version. How do you think?

@zhiics
Copy link
Member Author

zhiics commented Oct 2, 2019

@icemelon9 @yzhliu @kevinthesun I think we've probably forgotten this PR.

@junrushao
Copy link
Member

Is this ready for review?

@zhiics
Copy link
Member Author

zhiics commented Oct 2, 2019

@junrushao1994 yes

@yzhliu
Copy link
Member

yzhliu commented Oct 3, 2019

@yzhliu Yeah, I actually thought about replacing it as well. But I ended up skipping it because it looks there is not much difference. I think we probably want to keep compact memory as much as possible because auto broadcast will bring in the if_then_else guard which may not as performant as the compact version. How do you think?

so it won't work for those ops written in hybrid script, right?

@zhiics
Copy link
Member Author

zhiics commented Oct 3, 2019

@yzhliu Thanks. Removed the binding in hybrid script as it will go through lower anyway.

Copy link
Member

@icemelon icemelon left a comment

Choose a reason for hiding this comment

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

lgtm

@icemelon icemelon merged commit 9d5cba2 into apache:master Oct 11, 2019
@zhiics zhiics deleted the any branch October 11, 2019 17:44
anijain2305 pushed a commit to anijain2305/tvm that referenced this pull request Oct 17, 2019
* [tvm][any] broadcast with values other than 1

* Add test for incompatible runtime values

* Remove hybrid script compact buffer binding

* retrigger ci
wweic pushed a commit to neo-ai/tvm that referenced this pull request Oct 18, 2019
* [tvm][any] broadcast with values other than 1

* Add test for incompatible runtime values

* Remove hybrid script compact buffer binding

* retrigger ci
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.

6 participants