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

[Relay][ADT]Static Tensor Array #5103

Merged
merged 13 commits into from
Apr 5, 2020

Conversation

kevinthesun
Copy link
Contributor

@kevinthesun kevinthesun commented Mar 19, 2020

Add static tensor array for fixed rank tensor array. With this change, we can more easily do type inference and optimization for most tensor array use cases.

Thanks for @wweic working on the base infra of StaticTensorArrayOps class.

@wweic @zhiics @yongwww @icemelon9

@MarisaKirisame
Copy link
Contributor

There are lots of AST written by hand. Can you try using the relay parser?

@masahi
Copy link
Member

masahi commented Mar 19, 2020

Is the constraint only fixed "rank"? I have a use case for fixed "shape" tensor array (a tighter constraint)
https://discuss.tvm.ai/t/a-tensor-array-usage-question-stacking-a-list-of-fixed-sized-tensors/5565

@kevinthesun
Copy link
Contributor Author

kevinthesun commented Mar 20, 2020

There are lots of AST written by hand. Can you try using the relay parser?

@MarisaKirisame This PR follows the patten of current Tensor Array. What do you mean by using relay parser?

@kevinthesun
Copy link
Contributor Author

kevinthesun commented Mar 20, 2020

https://discuss.tvm.ai/t/a-tensor-array-usage-question-stacking-a-list-of-fixed-sized-tensors/5565

@masahi fixed shape is a subset of fixed rank and supported in this PR. For stack operator, even if we have all tensors in the array with the same static shape, the first axis output shape will still be Any().

@masahi
Copy link
Member

masahi commented Mar 20, 2020

@kevinthesun Great! I've been waiting for this (thanks @wweic). I think the first output axis of stack being Any makes sense and it is no problem for me, assuming it is the length of the array.

@MarisaKirisame This PR follows the patten of current Tensor Array. What do you mean by using relay parser?

I think Marisa is talking about writing the new ADT and functions in the "Relay language" itself and use the parser to make it available to python, similar to the way List ADT and its helper functions are implemented in Prelude.

@kevinthesun
Copy link
Contributor Author

I think Marisa is talking about writing the new ADT and functions in the "Relay language" itself and use the parser to make it available to python, similar to the way List ADT and its helper functions are implemented in Prelude.

I see. My understanding for this is List ADT primitives are small and mature enough to be implemented in prelude. We can also move tensor array related primitives into prelude when they becomes more mature. In addition, we might want to move the generic tensor array as well, not just static tensor array. Does this make sense?

@masahi
Copy link
Member

masahi commented Mar 20, 2020

Does this make sense?

Yes. Since the generic tensor array is already implemented in this way, I also think it is better to let this in first and later port all tensor array stuff to Relay lang. There could be common code between generic/static that should be refactored in the process.

@kevinthesun kevinthesun force-pushed the static-tensor-array-ops branch from fa9e4a7 to 92d7eaf Compare March 25, 2020 00:00
@kevinthesun kevinthesun force-pushed the static-tensor-array-ops branch from 8a4b4fc to 769cdbd Compare March 31, 2020 01:16
@wweic
Copy link
Contributor

wweic commented Mar 31, 2020

@MarisaKirisame The reason we can not use relay parser is that we want to dynamically generate the operators for specific shape while we convert the TF model to relay IR. Current relay text parser can not easily do that.

Comment on lines 41 to 43
shape_str = str(self.shape).replace('[', '').replace(']', '')\
.replace('(', '').replace(')', '').replace(', ', '_')\
.replace(',', '')
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should improve this a bit. can we use '_'.join(self.shape)?

Comment on lines 56 to 57
"""Defines the dynamic tensor ADT, which is the container for tensors
with variable shapes."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Defines the dynamic tensor ADT, which is the container for tensors
with variable shapes."""
"""Defines the static tensor ADT, which is the container for tensors
with fixed shape."""

python/tvm/relay/prelude.py Show resolved Hide resolved
lower = Var('lower', scalar_type('int32'))
upper = Var('upper', scalar_type('int32'))
tvar = Var('t')
case = Clause(PatternConstructor(self.get_var('tensor_constructor'), [PatternVar(tvar)]),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
case = Clause(PatternConstructor(self.get_var('tensor_constructor'), [PatternVar(tvar)]),
case = Clause(PatternConstructor(tensor_constructor, [PatternVar(tvar)]),

python/tvm/relay/prelude.py Show resolved Hide resolved
python/tvm/relay/prelude.py Show resolved Hide resolved
Function([x], Match(x, [case], False), tensor_type_var(), [])

def define_tensor_array_read(self):
"""Defines a function to get the head of a list. Assume the list has at least one
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Defines a function to get the head of a list. Assume the list has at least one
"""Defines a function to get the nth element of a list. Assume the list has at least one

list[tensor_t] -> Tensor[(Any), int32] -> tensor_t -> list[tensor_t]

Set static indices shape by specifying indices_shape.
Set for_update to get static indices shape operator.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Set for_update to get static indices shape operator.
Set force_update to get static indices shape operator.

@kevinthesun
Copy link
Contributor Author

@wweic Comments addressed. PTAL.

Copy link
Contributor

@wweic wweic left a comment

Choose a reason for hiding this comment

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

lgtm. some minor comments.

@@ -217,7 +218,7 @@ def define_tensor_array_unstack(self):
tensor_array_unstack_tensor(t) : tensor_t -> list[tensor_t]
"""
ndim = len(self.shape)
# Skip scalar case
# We don't register unstask for scalar tensor array
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# We don't register unstask for scalar tensor array
# We don't register unstack for scalar tensor array

python/tvm/relay/prelude.py Show resolved Hide resolved
python/tvm/relay/prelude.py Show resolved Hide resolved
@kevinthesun kevinthesun force-pushed the static-tensor-array-ops branch from 4a9dd1d to 183ac9e Compare April 3, 2020 00:50
@kevinthesun
Copy link
Contributor Author

@wweic Fixed.

@kevinthesun kevinthesun merged commit b5352ee into apache:master Apr 5, 2020
@kevinthesun
Copy link
Contributor Author

Thanks @wweic @masahi @MarisaKirisame

@kevinthesun kevinthesun deleted the static-tensor-array-ops branch April 5, 2020 20:42
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Apr 16, 2020
* Add other static tensor array ops

* Add tensor array get data

* Minor refactor

* Fix pylint

* Update docstring

* Make get data more generic

* Improve test

* Improve split test

* Improve get data

* Minor fix

* Further improvement for static shape

* Improve shape parsing

* Unify get_static_name
zhiics pushed a commit to neo-ai/tvm that referenced this pull request Apr 17, 2020
* Add other static tensor array ops

* Add tensor array get data

* Minor refactor

* Fix pylint

* Update docstring

* Make get data more generic

* Improve test

* Improve split test

* Improve get data

* Minor fix

* Further improvement for static shape

* Improve shape parsing

* Unify get_static_name
dpankratz pushed a commit to dpankratz/incubator-tvm that referenced this pull request Apr 24, 2020
* Add other static tensor array ops

* Add tensor array get data

* Minor refactor

* Fix pylint

* Update docstring

* Make get data more generic

* Improve test

* Improve split test

* Improve get data

* Minor fix

* Further improvement for static shape

* Improve shape parsing

* Unify get_static_name
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.

4 participants