-
Notifications
You must be signed in to change notification settings - Fork 74
Enable triton kernels matmul tests #5128
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
base: main
Are you sure you want to change the base?
Conversation
|
Is this for testing? If it is not ready for review, please convert to draft. |
84862d8 to
aafbe1a
Compare
5b2d42b to
990069a
Compare
| out_matmul_scale = out_matmul_scale.data.view(torch.uint8) | ||
| if has_scratchpad and "mx_out_scale" in memory["scratchpad"]: | ||
| out_matmul_scale = memory["scratchpad"]["mx_out_scale"] | ||
| out_matmul_has_mx = out_matmul_scale is not None and out_matmul.element_size() == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for changing this line of code. This currently matches the upstream version of this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because, in my opinion, it is a logical mistake. In this line (https://github.com/intel/intel-xpu-backend-for-triton/blob/c2a39f4396fcd5297a57b9326ffd788a29e82ef2/python/triton_kernels/triton_kernels/matmul_ogs.py#L294C10-L294C25 ), it always initializes with float32 if split_k > 2, and then element_size == 4. I think element_size is not the most suitable condition. It would make more sense to check if out_dtype equals 8 bits. Maybe it’s a bit confusing because I didn’t update the skiplist or remove the tests that were already fixed by these changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the skiplist. Also, all batched test cases were fixed in this Triton commit when we reland this commit all batched test cases should be resolved. However, there are still 16 test cases that need to be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please upstream the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added pr to upstream triton-lang/triton#8519
Signed-off-by: Witold Dziurdz <[email protected]>
990069a to
f51fe27
Compare
Fixes #5074