-
Notifications
You must be signed in to change notification settings - Fork 12.7k
sycl: Fix and disable more configurations of mul_mat #15151
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
Conversation
Still failing this case:
|
That's weird, I can definitely see this test passing on LunarLake iGPU and on PVC. Would you be able to have a closer look on your side? |
Yeah double checked. Failing exactly here:
Unfortunately, I do not have enough time to debug this. And thank you for taking a look into it. |
Are you using oneDNN? The other path may still be broken. The next step should be to remove oneMKL and oneMath so we may want to start disabling more configurations when oneDNN is not used if that's the reason why it's failing for you. |
Yes, it's the default using oneDNN |
@qnixsynapse I was able to reproduce the issue in a CI but not locally somehow. I don't know what is going on so I suggest to disable one specific configuration that is failing just to make the CI green. |
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.
LGTM for now.
But I think we need to fix all these cases sometime in the future. Unfortunately for me, the MUL_MAT code has become far too complex for me to even understand.
Yes, unfortunately I won't be able to contribute as much to this project and I don't know who will be able to fix these issues in the future. |
Follow up of PR #15092
The previous PR missed to fix some configurations.
This PR fixes one case with oneDNN but disabled another one (
type_a=f16,type_b=f32,m=1056,n=1,k=129,bs=[8,3],nr=[4,1],per=[0,2,1,3],v=0
). It should be possible to support it with the current oneDNN logic here but I'm not able to find a solution in a timely manner. I don't expect I will be able to work on it myself so feel free to continue this fix.This should be enough to make the SYCL CI green again. The PR also fixes some compilation warnings.