[ESIMD] Construct simd from simd_view using args deduction guide #4892
Conversation
Signed-off-by: Vyacheslav N Klochkov <[email protected]>
Signed-off-by: Vyacheslav N Klochkov <[email protected]>
|
The E2E test for this patch is this one: intel/llvm-test-suite#454 |
Signed-off-by: Vyacheslav N Klochkov <[email protected]>
| @@ -137,6 +137,12 @@ template <typename Ty, int N, class Derived, class SFINAE> class simd_obj_impl { | |||
| init_from_array(std::move(Arr)); | |||
| } | |||
|
|
|||
| /// Construct from simd_view object. | |||
There was a problem hiding this comment.
Please describe each template and constructor parameter in documentation comment. I know that some of the API lack that, but this should be the general rule, as the ESIMD spec is generated from the doxygen.
There was a problem hiding this comment.
(did not hit Finish Review yesterday, sorry)
There was a problem hiding this comment.
Fixed for the new constructors: b7bc4dc
| template <typename T1, | ||
| class = std::enable_if_t<detail::is_vectorizable_v<T1>>> | ||
| simd(T1 Val) : base_type((Ty)Val) { | ||
| __esimd_dbg_print(simd(T1 Val)); | ||
| } | ||
|
|
||
| /// Construct from simd_view. | ||
| template <int ViewedSimdLength, typename RegionT, |
There was a problem hiding this comment.
Thanks!
Seems like this is almost complete copy-paste of the simd_obj_impl variant. I missed that in the first review, unfortunately. Can duplication be somehow avoided? E.g. this simd variant just refers to the simd_obj_impl variant?
One more thing that I missed is simd_mask - it should also have this kind of constructor. I wonder if it is enough just to define that constructor in a generic way so that it is automatically inherited from simd_obj_impl by both simd and simd_mask w/o the need to define this constructor explicitly in them?
There was a problem hiding this comment.
Technically it can be deleted, I'll do that.
Need to add though that the deduction guide needs to be defined for 'simd' class, not for 'simd_obj_impl'.
Having something like this will now work for the assignment: "simd_view SV = V; simd V1 = SV":
// Deduction guide for 'simd_obj_impl' constructor from a 'simd_view' object.
// It helps to infer the template parameter 'N' of the class 'simd_obj_impl'.
template <typename T, int ViewedSimdLength, typename RegionT>
simd_obj_impl(simd_view<simd<T, ViewedSimdLength>, RegionT>) -> simd_obj_impl<T, RegionT::length, simd<T, RegionT::length>>;
There was a problem hiding this comment.
One more note: the mentioned 'simd' constructor can be deleted only because of this statement in simd class:
using base_type::base_type;
The same for 'simd -> simd_view' is not doable because 'simd_view' does not have such 'using' statement and the following ctor cannot be removed:
https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/ext/intel/experimental/esimd/simd_view.hpp#L76
There was a problem hiding this comment.
Regarding the 'simd_mask' and doing the arguments deduction for it...
Currently, 'simd_mask' is declared as an alias template, not as a template class and thus the template args deduction guides do not work for it.
simd_mask<16> m16 = 0;
auto mask_view8 = m16.select<8, 1>(0);
detail::simd_mask_impl m8 = mask_view8; // Works well with some changes in headers
simd_mask m8 = mask_view8; // Does not work. The error is: "error: alias template 'simd_mask' requires template arguments; argument deduction only allowed for class templates"
I don't know why simd_mask is not defined as a class instead of an alias to a class in detail namespace.
Anyway, tackling this problem for simd_mask deserves a separate PR, IMO.
Signed-off-by: Vyacheslav N Klochkov <[email protected]>
This PR does not seem to have a test case for simd construction from a simd_view - am I missing something? |
The line 139 of the test there: "simd vb = vb_view;" tests this new constructor + deduction guide. That test fails without this PR. See the log: http://icl-jenkins.sc.intel.com:8080/blue/organizations/jenkins/LLVM-Test-Suite-CI-TMP%2FLLVM-Test-Suite-CI-Linux/detail/LLVM-Test-Suite-CI-Linux/908/pipeline |
I see, thanks for the pointer. |
intel/llvm#4514 Signed-off-by: Vyacheslav N Klochkov <[email protected]>
|
I have recently watched SIMD in C++20 - EVE of a new Era - Joël Falcou - CPPP 2021 on https://github.com/jfalcou/eve by @jfalcou and there are a lot of interesting design ideas which might be useful in some SYCL SIMD 1D & 2D extensions. |
Thanks, can be helpful indeed. |
Signed-off-by: Vyacheslav N Klochkov [email protected]