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

[i1] Do not emit arith.trunci cast from i1 to i1 #19176

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lialan
Copy link
Contributor

@lialan lialan commented Nov 17, 2024

arith.trunci does not allow cast to same type

@lialan
Copy link
Contributor Author

lialan commented Nov 17, 2024

@rohan-tan-bhowmik I am trying to fix an issue while making i1 attention works. However I have a problem: I don't know where to find an minimized test case for it as after decomposition my simple test cases are turned into big ones. Original commit does not show a lot of helpful info as well.

I am wondering if you can give me a pointer to where I can get a test case for it?

@rohan-tan-bhowmik
Copy link
Member

Hi @lialan! I had created a repository for specifically testing different variants of masked and causal attention: https://github.com/rohan-tan-bhowmik/iree-masked-attention-test. Please check it out and let me know if you have any questions about it. If this doesn't seem to help, let me know as well :)

@lialan
Copy link
Contributor Author

lialan commented Nov 17, 2024

Hi @lialan! I had created a repository for specifically testing different variants of masked and causal attention: https://github.com/rohan-tan-bhowmik/iree-masked-attention-test. Please check it out and let me know if you have any questions about it. If this doesn't seem to help, let me know as well :)

@rohan-tan-bhowmik Thanks! I am actually already using your repo to test i1 attention support. I already hit a problem: the emitted iree_linalg_ext.attention now needs to take a region which yields values. But the script does not append a region for it.

I am doing manual changes in my private repo to bypass it, but it would be great to amend that in your repo!

@lialan
Copy link
Contributor Author

lialan commented Nov 17, 2024

@rohan-tan-bhowmik still, the generated test file is too big a unit test for such a small change. I wonder if you can help me find a unit test for it?

@rohan-tan-bhowmik
Copy link
Member

@lialan Hmm, would #17892 help?

@lialan
Copy link
Contributor Author

lialan commented Nov 17, 2024

@lialan Hmm, would #17892 help?

@rohan-tan-bhowmik
It seems currently it emits attention test but the attention test is missing the optional i1 masking component? I can add one new component to it, it is simple.

Also perhaps I can still use a unit test here? But I think this change is small, I am okay submitting this PR without a test case.

@lialan
Copy link
Contributor Author

lialan commented Nov 17, 2024

I can compile the out-of-tree repo emitted test with some changes along with this PR and another PR upstream. But it is missing correctness checking mechanism, and also the mask.

Need this test to verify the generated result is sound, looking into it.

@lialan lialan marked this pull request as ready for review November 19, 2024 06:43
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