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

Support more ops in dot hoisting #14

Open
wants to merge 3 commits into
base: llvm-head
Choose a base branch
from

Conversation

acollins3
Copy link

Add support for hoisting dot layouts through arith.truncf, arith.trunci and math ops.

Note: I have some test cases (written as triton python examples) but wasn't sure whether you'd want these included in the PR. I am seeing a lot of test failures when running the triton tests on llvm-head with the openxla specific changes.

@Moerafaat
Copy link
Member

The diff here includes changes that are made from exporting patches that we maintain. It's very hard to review the actual changes at the moment. Can you please rebase on top of the latest commit in llvm-head? Currently it is this one 3596dc5

@chsigg
Copy link
Member

chsigg commented Sep 23, 2024

Maybe best if you create PRs against the main branch. That way it will be easier to upstream.

@acollins3
Copy link
Author

I've rebased the changes on the latest llvm-head branch. Rebasing onto main wasn't really possible, as these changes build on what is in llvm-head.

@Moerafaat
Copy link
Member

The ops that are filtered in this code probably didn't work for a reason. A while back we added Select Op as it also faced problems when propagating the dot_op layout to the mask. I would expect that removing the Trunc Ops would require fixes to make them work. Did this "just work" after removing the condition?

Ofc we will need test cases that cover this. Particularly lit tests showing the propagation when the Hoisting pattern activates. We also need to see that the tests lower correctly to LLVM. This might require an e2e test (or if it is already covered by an existing test we would require you to highlight that test/s).

@Moerafaat
Copy link
Member

Ops, seems that updates we have to maintain the Triton integration will cause PR diffs to break because of force-updates. We might need to figure out a better way to handle this as we didn't intend for this repo to accept incoming PRs. Apologies for this, but you will need to rebase 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.

4 participants