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

[RFC][Backend] RFC-CSI-NN2-Integration #75

Merged
merged 3 commits into from
Jun 15, 2022
Merged

Conversation

alter-xp
Copy link
Contributor

@alter-xp alter-xp commented May 30, 2022

Introduce CSI-NN2 Compute Library into TVM to accelerate the inference performance of RISC-V CPU with Vector Extension.

@areusch

@hogepodge
Copy link
Contributor

This is fantastic, thank you! We're excited to hear about the results for MLPerf. My main comment is mostly concerned about documentation. I'm glad the build instructions are included with the RFC, but I'd like to see the inclusion of documentation about how to configure, build, and use the platform be explicitly updated and included.

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

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

@alter-xp thanks for the rfc! I added a couple questions for you, but overall this looks pretty good.

rfcs/0075_RISC-V_CSI-NN2_Intergration.md Show resolved Hide resolved
rfcs/0075_RISC-V_CSI-NN2_Intergration.md Outdated Show resolved Hide resolved
rfcs/0075_RISC-V_CSI-NN2_Intergration.md Show resolved Hide resolved
rfcs/0075_RISC-V_CSI-NN2_Intergration.md Outdated Show resolved Hide resolved
@alter-xp
Copy link
Contributor Author

alter-xp commented Jun 6, 2022

This is fantastic, thank you! We're excited to hear about the results for MLPerf. My main comment is mostly concerned about documentation. I'm glad the build instructions are included with the RFC, but I'd like to see the inclusion of documentation about how to configure, build, and use the platform be explicitly updated and included.

Thanks for comments. we provide a documentation in PR for TVM. But I'm not sure whether these contents need to be completely written in RPC

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

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

thanks @alter-xp , couple more follow-ups here

rfcs/0075_RISC-V_CSI-NN2_Intergration.md Outdated Show resolved Hide resolved
rfcs/0075_RISC-V_CSI-NN2_Intergration.md Show resolved Hide resolved
@alter-xp
Copy link
Contributor Author

ok, then for CI do you plan to e.g. expand our ci_qemu Docker image to additionally contain this custom qemu? (this involves committing a change to docker/, then pinging a committer to update the version of the image used)

Thanks for advice, we will add it. but I have no experience about this. Is there any relevant process?

@areusch
Copy link
Contributor

areusch commented Jun 10, 2022

@alter-xp we have some:

we need to write that up :/

the basics are:

  1. submit a PR which only modifies docker/ scripts. the CI will rebuild docker containers and then run through using the newly-rebuilt containers.
  2. ping a committer to publish the built images to the tlcpack dockerhub org
  3. submit another PR which modifies jenkins/Jenkinsfile.j2 (and run generate.py there to modify Jenkinsfile) to reference the newly-pushed containers. you can also add tests which depend on the newly-added dependencies here.

@alter-xp
Copy link
Contributor Author

@areusch Thanks a lot. let me update the image.

Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

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

thanks @alter-xp ! @kparzysz-quic any final comments? i'll merge in a day if i don't hear anything.

@areusch areusch merged commit cfcf114 into apache:main Jun 15, 2022
@areusch
Copy link
Contributor

areusch commented Jun 15, 2022

thanks @alter-xp, the PR is now merged! please open an RFC tracking issue and list out the PRs we can expect there, then we can proceed on the CI changes.

@alter-xp
Copy link
Contributor Author

@areusch a tracking issue is ready. I will keep updating it.

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.

4 participants