[SYCL] Use mp11 to sort compile-time properties #8168
Conversation
Currently, property lists are sorted using a hand-written implementation of merge-sort. Using mp11, makes it more efficient and improves code readability.
| // Using mp11 to sort property lists. | ||
| namespace __MP11_NS = sycl::detail::boost::mp11; |
There was a problem hiding this comment.
I think it's better not to introduce that in the global namespace.
| // Sort types accoring to their PropertyID. | ||
| struct SortByPropertyId { | ||
| template<typename T, typename U> using fn = | ||
| __MP11_NS::mp_bool<PropertyID<T>::value < PropertyID<U>::value>; |
There was a problem hiding this comment.
... i.e, use fully qualified name here,
| using type = std::tuple<LTs...>; | ||
| // Sort types accoring to their PropertyID. | ||
| struct SortByPropertyId { | ||
| template<typename T, typename U> using fn = |
There was a problem hiding this comment.
Can we use the same T1/T2 as on the left? I'm still not sure what our policies regarding those identifiers are.
After integrating Boost::mp11, sycl::detail::something is ambigous so compiler prints the fully-qualified name, for e.g.: sycl::_V1::detail::something.
There was a problem hiding this comment.
LGTM, the tests changes are due to the change in dumps that are non-functional, similar to what can be seen at https://godbolt.org/z/ndan4djdb
@steffenlarsen , do you agree with this? An alternative would be to update our boost machinery to use sycl::_V1::detail::boost instead of current sycl::detail::boost.
I am okay with either, though I prefer the one that's most unlikely to cause the need to change the test in the future. Would the latter localize it to the actual changes? Note also that RHEL build is failing, seemingly due some new ambiguities in unchanged code. First issue I see is use of |
This reverts commit a5259be.
…cl::detail namespace This change is required to avoid compiler ambiguity between sycl::detail::<something> and sycl::_V1::detail::<something> when mp11 header is included in kernel_properties.cpp.
|
Thanks for the feedback @steffenlarsen and @aelovikov-intel . Instead of my previous changes in the test cases, I have now made changes to import boost into sycl::_V1::detail namespace instead of the sycl::detail namespace. This prevents the need for updating test cases and also fixes the errors in pre-commit testing (jenkins / SYCL_CI/intel/Build_PR_RHEL / Build_PR_RHEL / #18327). |
|
@llvm-gatekeepers The PR is ready! |
This reverts commit eb31c2d.
Commit causes `sycl::detail` ambiguity errors with MSVC versions. Reverts #8168
This PR reintroduces #8168 that was reverted by #8325 due to some build errors with MSVC compilers. Upon inspection, it was found that the build errors occur only in the case of incremental builds. With clean builds - i.e. completely removing the build directory and re-configuring/re-building again - there won't be any errors. -------------- Currently, compile-time properties are sorted using a hand-written implementation of merge-sort. However, this PR propose to sort compile-time properties using Boost's MP11 metaprogramming library instead. Mp11's sorting functionality is more optimized and it also improves the overall code readability (83 LOC vs just 11 LOC). PS:- This PR does not introduce any new dependency as Mp11 is already integrated into the code base (See #5791).
Currently, compile-time properties are sorted using a hand-written implementation of merge-sort. However, this PR propose to sort compile-time properties using Boost's MP11 metaprogramming library instead. Mp11's sorting functionality is more optimized and it also improves the overall code readability (83 LOC vs just 11 LOC).
PS:- This PR does not introduce any new dependency as Mp11 is already integrated into the code base (See #5791).