-
Notifications
You must be signed in to change notification settings - Fork 798
[SYCL] Remove deprecated get_backend_info entry points
#20676
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
get_backend_info ABI entry pointsget_backend_info entry points
|
@KseniyaTikhomirova Could you please take a look. |
| template <typename Param> | ||
| typename detail::is_backend_info_desc<Param>::return_type | ||
| get_backend_info() const; | ||
| get_backend_info() const; |
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's a bit surprising that this remains. Maybe link original PR in the description to explain what is happening.
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.
This was implemented in #12906 and deprecated in #16700.
As far as I understand get_backend_info is documented in the spec: https://registry.khronos.org/SYCL/specs/sycl-2020/html/sycl-2020.html#_information_query_interface but corresponding descriptors are supposed to be specified in backend-specific extensions. At the same time we don't have any such backend-specific descriptor right now and sycl-cts doesn't seem to test this. So I am not sure if we should leave these methods in place or remove them.
@HPS-1 @AlexeySachkov can you provide your input please?
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's a bit surprising that this remains. Maybe link original PR in the description to explain what is happening.
there is no harm to have this entry point while
template <typename T> struct is_backend_info_desc : std::false_type {};
is still present.
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 but I will leave approval to Andrey to ensure that his concerns are resolved.
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 think that our plan was to just leave them in place as a foundation for when we actually get those extensions.
Having them would also result in a better error message for user, i.e. "something is not a valid info that can be queried through get_backend_info vs this method does not exist" - the latter may be perceived as a bug by an end user
|
windows timeout issue is unrelated as it was visible in pre-commit testing for other merged prs. |
This was implemented in intel#12906 and deprecated in intel#16700. See the latter for reasoning.
This was implemented in #12906 and deprecated in #16700. See the latter for reasoning.