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

Add a CPU lowering pass #26

Merged

Conversation

shintaro-iwasaki
Copy link
Contributor

@shintaro-iwasaki shintaro-iwasaki commented Nov 1, 2023

Per discussion in #24, this PR creates a reference-CPU backend for triton-shared using the standard MLIR->LLVM lowering passes.

  • Without any changes in the Triton runtime, we can run triton-shared on CPU from Python-Triton code.
    • Potentially it is useful for testing.
  • A very basic kernel (see reduce.py) runs and succeeds.
  • Many kernels do not run because of a lack of LLVM lowering and other problems.
    • For example, memref::tensorStore->LLVM seems not implemented, so many kernels are not supported at this point.

Help from the community and triton-shared core developers is needed to improve and maintain it. I'd appreciate your feedback and suggestions.

Note: compared with the PR I used in the discussion, this PR version can run a compute kernel (reduce.py, as an example) by using MLIR's CRunnerUtils.h.

(EDIT: let me understand if I can sign this CLA)

@nhat-nguyen
Copy link
Collaborator

Thank you so much @shintaro-iwasaki! I'll start taking a look 😄

Copy link
Collaborator

@nhat-nguyen nhat-nguyen left a comment

Choose a reason for hiding this comment

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

@shintaro-iwasaki I left a few nit comments, other than that, everything looks good! Thanks again for contributing this. @manbearian mentioned that he couldn't get tensor store to work, so I'll be investigating that separately once we merge this.

python/__init__.py Outdated Show resolved Hide resolved
python/__init__.py Outdated Show resolved Hide resolved
python/__init__.py Outdated Show resolved Hide resolved
python/__init__.py Outdated Show resolved Hide resolved
python/__init__.py Outdated Show resolved Hide resolved
python/__init__.py Outdated Show resolved Hide resolved
python/__init__.py Outdated Show resolved Hide resolved
python/ExecutionEngine/version.txt Show resolved Hide resolved
.github/workflows/integration-tests.yml Show resolved Hide resolved
@nhat-nguyen
Copy link
Collaborator

@kshama-msft Would you mind helping Shintaro with the CLA question? Thank you!

@kshama-msft
Copy link

@shintaro-iwasaki if the CLA has been generated by the github bot then feel free to sign it. You should review with your internal open-source contribution legal teams to confirm that it is okay for you to sign.

@shintaro-iwasaki
Copy link
Contributor Author

Thanks a lot for your review, @nhat-nguyen!
I am still figuring out the CLA thing with the legal team. Please give me some more time. Once it's cleared, let me fix all the issues pointed out.

@shintaro-iwasaki
Copy link
Contributor Author

@microsoft-github-policy-service agree [company="Meta Platforms, Inc."]

@shintaro-iwasaki
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Meta Platforms, Inc."

@shintaro-iwasaki
Copy link
Contributor Author

The legal thing has been cleared on my side. Let me fix the issues pointed out and update the PR by the end of today.

@shintaro-iwasaki
Copy link
Contributor Author

Thanks! I updated the commit as well as rebased onto the latest main. Please let me know any further suggestions!

(Do you want the PR author (me) to resolve the comments? Or should it be done by reviewers?)

@nhat-nguyen
Copy link
Collaborator

Thanks! I updated the commit as well as rebased onto the latest main. Please let me know any further suggestions!

(Do you want the PR author (me) to resolve the comments? Or should it be done by reviewers?)

Thank you @shintaro-iwasaki, everything looks good! We usually let the PR authors resolve the comments but let me know if you'd prefer otherwise. Once the pipeline runs finish, I can merge in the changes. 😄

@nhat-nguyen nhat-nguyen self-requested a review November 6, 2023 21:58
@nhat-nguyen nhat-nguyen merged commit 6fa7ce3 into microsoft:main Nov 6, 2023
3 checks passed
@nhat-nguyen
Copy link
Collaborator

Merged! Thank @shintaro-iwasaki again!

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.

None yet

3 participants