-
Notifications
You must be signed in to change notification settings - Fork 743
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
[SYCL][NFCI] Finalize switch to SPV_KHR_cooperative_matrix #16045
Conversation
Signed-off-by: Sidorov, Dmitry <[email protected]>
Signed-off-by: Sidorov, Dmitry <[email protected]>
Tested locally on linux both CPU and GPU - the patch shouldn't cause any regressions (unless there is a difference between driver versions used in pre-commit here in CI and drivers used locally (setup by scripts)). |
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.
Code changes itself LGTM.
@MrSidims , in addition to stability, could you please also verify performance just in case to make sure no performance regressions are introduced?
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.
Let's investigate failures on Windows DG2 before merging this
@dkhaldi on windows DG2 only these 2 tests are failing: But there are indeed regressions. joint_matrix_out_bounds.cpp was added only for JointMatrices, with the switch it's actually failing. The reason is that __spirv_CooperativeMatrixLoadCheckedINTEL (and Store) are not handled by IGC for Cooperative Matrices at all. This behavior doesn't follow SPIR-V specification as it defines OpCooperativeMatrixLoadCheckedINTEL be used only for OpTypeCooperativeMatrixKHR. Support of TypeJointMatrixINTEL was left in the translator only as an exception for the switch. Prefetch seem to also not handled by IGC when used with TypeCooperativeMatrixKHR. We have to postpone the switch to cooperative matrices as well as promoting the sycl extension from experimental to supported. Note, only new functionality doesn't work. Old functionality works as expected. |
Closing the PR as we can't merge it |
@dkhaldi @YuriPlyakhin apparently I had a problem with the driver. On agama driver performance tests are passing (with OOB loads/stores). No performance regressions observed. |
@dkhaldi @intel/llvm-reviewers-runtime please take a look |
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! Let's embrace for impact :)
@intel/llvm-reviewers-runtime friendly ping to review the SYCL headers and probably 'unsupported feature' test(s) changes. |
@MrSidims Looks like this broke some build lit tests, can you take a look?
|
|
Cool, thx! |
No description provided.