-
Notifications
You must be signed in to change notification settings - Fork 730
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
[NFCI][SYCL] Simplify property_key creation #12831
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
struct buffer_location_key | ||
: oneapi::experimental::detail::compile_time_property_key< | ||
oneapi::experimental::detail::PropKind::BufferLocation> { |
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.
Would that be always ABI compatible taking into account that some properties are used in sycl/source? E.g.: https://github.com/intel/llvm/blob/sycl/sycl/source/detail/usm/usm_impl.cpp#L98-L99
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.
The ones in that example are from SYCL 2020 properties, while these properties are all from the sycl_ext_oneapi_properties extension. Since that property-list is templated I don't believe it crosses the library boundaries, so it would only be if we pass the properties around by themselves.
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.
ESIMD part is good.
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.
Generally I think this is a reasonable, but worry about exposing non-standard implementation details publicly.
struct buffer_location_key | ||
: oneapi::experimental::detail::compile_time_property_key< | ||
oneapi::experimental::detail::PropKind::BufferLocation> { |
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.
The ones in that example are from SYCL 2020 properties, while these properties are all from the sycl_ext_oneapi_properties extension. Since that property-list is templated I don't believe it crosses the library boundaries, so it would only be if we pass the properties around by themselves.
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.
Neat! 👍
intel#12831 inadverdently removed the detail namespace opening from the GRF size property header. This commit returns this namespace. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
#12831 inadverdently removed the detail namespace opening from the GRF size property header. This commit returns this namespace. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
No description provided.