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

[Frontend][TensorFlow]TensorFlow Parser Control Flow Enhancement #5020

Merged
merged 14 commits into from
Mar 23, 2020

Conversation

kevinthesun
Copy link
Contributor

RFC: #4969
In addition to control flow improvements, this PR also changes some operator parsing logic to support symbolic input for certain attributes.

@zhiics @yongwww @jroesch @srkreddy1238 @masahi

@kevinthesun kevinthesun requested a review from zhiics March 10, 2020 00:53
@kevinthesun kevinthesun force-pushed the TensorFlowControlFlow branch 3 times, most recently from 31816bf to 81f6d17 Compare March 11, 2020 16:59
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.

Some initial comment. Need to read it one more time.

cc @gmagogsfm who might be also interested.

def b(i): return [tf.add(i, 1), tf.add(i, 1) + slice]
r = tf.while_loop(c, b, [i])

If we directly create recursive function, slice will be placed into function body.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we need to add a loop-invariant code motion (LICM) pass so that we don't need to handle this in different frameworks. It can benefit general Relay program as well, though it is not necessarily in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. In the future we can add a pass automatically detect invariant part inside loop/function, I'll add a TODO here.

"""Find name of direct parent while loop."""
ploop_name = ""
name_prefix = node_name.rsplit('/', 1)[0]
if name_prefix.startswith("^"):
Copy link
Member

Choose a reason for hiding this comment

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

Why start with "^"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In certain cases, some nodes under while block will start will ^ sign, but they are still under the same while loop.

name_prefix = node_name.rsplit('/', 1)[0]
if name_prefix.startswith("^"):
name_prefix = name_prefix[1:]
for lname in self._while_loop_name_set:
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment to explain how the parent and child relationship is defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

def visit(self, expr):
"""
For each expression in the body, look up the corresponding
tensorflow node with its structural hash. If the current loop is the
Copy link
Member

Choose a reason for hiding this comment

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

TensorFlow

@kevinthesun
Copy link
Contributor Author

Since a tensorflow opdilation2d is added to tf parser, I need to rebase and add mod argument to it.

@kevinthesun kevinthesun force-pushed the TensorFlowControlFlow branch from 5e58143 to 89cbe41 Compare March 17, 2020 20:26
@kevinthesun
Copy link
Contributor Author

@zhiics PTAL

@zhiics
Copy link
Member

zhiics commented Mar 17, 2020

@kevinthesun sorry, will do by tomorrow, too much stuff recently.

@kevinthesun kevinthesun force-pushed the TensorFlowControlFlow branch 2 times, most recently from 449138c to 97ba277 Compare March 18, 2020 23:55
@kevinthesun
Copy link
Contributor Author

@srkreddy1238 Could you also take a look?

@kevinthesun kevinthesun force-pushed the TensorFlowControlFlow branch 2 times, most recently from f987edd to 3a1130f Compare March 23, 2020 00:10
@kevinthesun kevinthesun force-pushed the TensorFlowControlFlow branch from 3a1130f to f8de9a7 Compare March 23, 2020 02:48
@zhiics zhiics merged commit 7bc0b27 into apache:master Mar 23, 2020
@zhiics
Copy link
Member

zhiics commented Mar 23, 2020

Thanks @kevinthesun

@kevinthesun kevinthesun deleted the TensorFlowControlFlow branch March 23, 2020 17:32
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Apr 16, 2020
…che#5020)

* Improve TF control flow major logic

* Pass mod into operator convert function

* Fix LoopBound

* Add more control flow tests

* Add two test cases for stridedslice

* Fix docstring

* Fix lint

* Fix import

* Fix test assert

* Minor fix conv3d

* Add more comments

* Fix for dilation2d

* Change newly added atan

* Change newly added unravel
zhiics pushed a commit to neo-ai/tvm that referenced this pull request Apr 17, 2020
…che#5020)

* Improve TF control flow major logic

* Pass mod into operator convert function

* Fix LoopBound

* Add more control flow tests

* Add two test cases for stridedslice

* Fix docstring

* Fix lint

* Fix import

* Fix test assert

* Minor fix conv3d

* Add more comments

* Fix for dilation2d

* Change newly added atan

* Change newly added unravel
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