Skip to content

Conversation

@gh10242048
Copy link

If reshape in our relay expression, like reshape + matmul expr, when we use set_input_zero_copy or set_output_zero_copy,the result goes wrong.

@tvm-bot
Copy link
Collaborator

tvm-bot commented Jun 19, 2023

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.

  • No users to auto-tag found, no teams are specified in PR title See #10317 for details

Generated by tvm-bot

Copy link
Contributor

@echuraev echuraev left a comment

Choose a reason for hiding this comment

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

Could you please add correspondent unit tests?

@gh10242048 gh10242048 closed this Jun 19, 2023
@gh10242048
Copy link
Author

Could you please add correspondent unit tests?
ok

@gh10242048 gh10242048 reopened this Jun 19, 2023
@gh10242048 gh10242048 requested a review from echuraev June 21, 2023 03:38
Copy link
Contributor

@echuraev echuraev left a comment

Choose a reason for hiding this comment

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

A couple of minor comments. @vinx13, @yinghai could you please review?


if (param.func_name == "__nop") {
return {[]() {}, arg_ptr};
return {[arg_ptr]() {}, arg_ptr};
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason of capturing arg_ptr in empty lambda?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, I would say that better use zero instead of 0 in the name of the file, e.g test_reshape_zero_copy.py.
Also, probably you can move this test to the file with existing tests on zero_copy.

Copy link
Contributor

@echuraev echuraev left a comment

Choose a reason for hiding this comment

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

In general LGTM. Just a couple of moments which were described in previous comment.

@echuraev echuraev requested a review from vinx13 June 21, 2023 05:55
@corehalt
Copy link
Contributor

@echuraev , cc: @masahi
Hi, can we please merge this? current set_output_zero_copy doesn't work without these changes when we have a (reshape) nop at the end of a graph (it prevents to forward the pointer of the actual output area through the nop) 🙏

Thank you!

@corehalt
Copy link
Contributor

oh also @vinx13 was involved sorry, forgot to mention

@masahi
Copy link
Member

masahi commented Jul 10, 2024

@corehalt The PR needs to resolve the conflict. Feel free to take over this PR and create a new one.

@corehalt
Copy link
Contributor

@masahi Thank you, I will try to find sometime to send a new one.
Most of the contents in this PR looks fine to you except for the feedback of @echuraev and the conflict?

@masahi
Copy link
Member

masahi commented Jul 10, 2024

Yes it's fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants