-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[OPENCL][TEXTURE] Test case enhancements and fixes for RPC #13408
Conversation
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment. Generated by tvm-bot |
1fcea3e
to
7fd4b6e
Compare
RPC execution fails some times when opened multiple times per test case. Making remote object initialized from pytest session instead. Network test cases added. dtypes enhanced to support multiple inputs.
7fd4b6e
to
86bb810
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good improvements! Thank you. One question
@@ -1549,7 +1549,7 @@ def convert_gather(self, op): | |||
assert axis < data_dim, "Axis out of bounds" | |||
|
|||
if self.has_expr(indices.tensor_idx): | |||
indices_expr = self.get_expr(indices.tensor_idx) | |||
indices_expr = _op.cast(self.get_expr(indices.tensor_idx), "int32") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this _op.cast
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a bug I encountered while importing other tflite models.
Those models not added here as they are time consuming, but planning to add them to benchmarking scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leandron, @AndrewZhaoLuo, @masahi I have doubts, should the test be added for this change or not?
Other changes LGTM.
bb7a604
to
e5884cc
Compare
079a2d1
to
c5d0e51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks
acd25b9
to
c5d0e51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'm not 100% sure, but seems like since the merge of the PR, the testcase below consistently fails for some reason:
i can submit a PR to temporarily skip the test, but would you guys mind taking a look? |
|
RPC execution fails some times when opened multiple times per test case. Making remote object initialized from pytest session instead.
Network test cases added.
dtypes enhanced to support multiple inputs.