-
Notifications
You must be signed in to change notification settings - Fork 39
Remove non spirv builtins #455
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: sycl-develop
Are you sure you want to change the base?
Conversation
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, couple of additional suggestions
if (CMAKE_CXX_COMPILER_ID MATCHES "IntelLLVM" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 2025.2) | ||
list(APPEND DPCPP_FLAGS "-Xspirv-translator;-spirv-ext=+SPV_INTEL_split_barrier") |
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 does this mean for someone using 2025.1? If it means that Xe code won't work (because there are no non-spirv builtins exposed), we should probably change this to an assertion/error or something?
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.
No, this works with 2025.1 and new enough driver.
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.
OK can you explain what the logic is here then? For 2025.1
, this first branch (line 66) will be hit, so we won't have extensions +SPV_INTEL_2d_block_io,+SPV_INTEL_subgroup_matrix_multiply_accumulate
. Are those flags not needed to expose SPIR-V builtins on 2025.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.
It seems they are not. I tested this locally and it works.
To be honest I just tried to keep this as similar to how it was before, but I do not fully understand which extensions are needed in which case. @aacostadiaz might know more.
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.
You can remove the if condition entirely keeping the +SPV_INTEL_2d_block_io,+SPV_INTEL_subgroup_matrix_multiply_accumulate modules which are supposed to be required
Removes the option to use non spir-V builtins to implement MMA and copy atoms. This removes the burden of maintaining two sets of builtins for implementing atoms.