Skip to content

Conversation

@zhuwenxi
Copy link
Contributor

Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

Thanks for the RFC and overall LGTM. One suggestion I have is that it would be great if you could provide an upstream plan that briefly explains how would you send PRs. To facilitate the review process, it is encouraged to break down your implementation to a series of small PRs. Here is an example of a PR series:

  1. Add libxsmm to the TVM CI.
  2. Add libxsmm to TOPI.
  3. Add libxsmm to Relay op strategy.
  4. Add libxsmm to BYOC.

@zhuwenxi
Copy link
Contributor Author

Thanks for the RFC and overall LGTM. One suggestion I have is that it would be great if you could provide an upstream plan that briefly explains how would you send PRs. To facilitate the review process, it is encouraged to break down your implementation to a series of small PRs. Here is an example of a PR series:

  1. Add libxsmm to the TVM CI.
  2. Add libxsmm to TOPI.
  3. Add libxsmm to Relay op strategy.
  4. Add libxsmm to BYOC.

Upstream plan added, at the end of RFC.

By the way, I'm not quite sure what "TVM CI" refers to? If it means unit tests, they will be included in their related PRs when I upstream my code.

@zhuwenxi zhuwenxi changed the title RFC to integrate LIBXSMM with TVM. [RFC] Integrate LIBXSMM with TVM. Dec 18, 2021
@zhuwenxi
Copy link
Contributor Author

@comaniac Any update?

Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Just try to polish the plan for the last time and we should be good to go.

@zhuwenxi
Copy link
Contributor Author

@comaniac Thank you, I've updated the plan, please let me know if there is still problems.

@comaniac comaniac merged commit 1a3d4f1 into apache:main Dec 24, 2021
@comaniac
Copy link
Contributor

Thanks @zhuwenxi this is now merged.
Also please file a follow-up PR to add the RFC information (start date, RFC PR and RFC tracking issue). You can refer to other merged RFCs for details.

@zhuwenxi
Copy link
Contributor Author

@comaniac OK, I'll update the RFC information soon.

@zhuwenxi
Copy link
Contributor Author

@comaniac I'm starting to implement the first PR "Add libxsmm to TVM CI" recently. I wonder if there is any CI-related PR I can refer to?

@comaniac
Copy link
Contributor

@comaniac I'm starting to implement the first PR "Add libxsmm to TVM CI" recently. I wonder if there is any CI-related PR I can refer to?

You could refer to the PR like apache/tvm#9881 or something similar.

@zhuwenxi
Copy link
Contributor Author

@comaniac I'm starting to implement the first PR "Add libxsmm to TVM CI" recently. I wonder if there is any CI-related PR I can refer to?

You could refer to the PR like apache/tvm#9881 or something similar.

Thank you!

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.

2 participants