[ESIMD][NFC] Remove constexpr from simd class ctors #3944
Conversation
| template <typename SrcTy> simd(const simd<SrcTy, N> &other) { | ||
| if constexpr (std::is_same<SrcTy, Ty>::value) | ||
| set(other.data()); | ||
| else | ||
| set(__builtin_convertvector(other.data(), detail::vector_type_t<Ty, N>)); | ||
| } |
There was a problem hiding this comment.
Not something you changed as part of this PR, but something I hope can be fixed (maybe as part of a different PR).
This constructor uses bad practice of how to deal with a templated constructor which may or may not convert. The problem with this way is that it messes with traits like is_trivially_constructible. Rather the converting and non-converting should be different overloads. This should be done as:
| template <typename SrcTy> simd(const simd<SrcTy, N> &other) { | |
| if constexpr (std::is_same<SrcTy, Ty>::value) | |
| set(other.data()); | |
| else | |
| set(__builtin_convertvector(other.data(), detail::vector_type_t<Ty, N>)); | |
| } | |
| simd(const simd&) = default; | |
| template <typename SrcTy> simd(const simd<SrcTy, N> &other) { | |
| set(__builtin_convertvector(other.data(), detail::vector_type_t<Ty, N>)); | |
| } |
The non-templated is picked over the templated and will therefore be used in the non-converting case. And because it is defaulted, things like traits work correctly.
| else | ||
| set(__builtin_convertvector(other.data(), detail::vector_type_t<Ty, N>)); | ||
| simd() = default; | ||
| simd(const simd &other) { set(other.data()); } |
There was a problem hiding this comment.
@rolandschulz suggested to make it default too, which makes sense unless there are unforeseen complications like bad code gen. Are there any? If yes, please add a comment. Otherwise, =default should work.
There was a problem hiding this comment.
Yes, but set is calling __esimd_vstore for device compilation. Let me double-check.
There was a problem hiding this comment.
@rolandschulz, @DenisBakhvalov - this patch luckily fixes a real problem. constexpr leads to creating a constant global of simd wrapper type, e.g:
@vc = private unnamed_addr addrspace(1) constant %"esimd::simd" \{ { <16 x i16> <i16 1, i16 1, i16 1, i16 1, i16 1, i16 1, i16 1, i16 1, i16 1, i16 1, i16 1, i16 1, i16 1, i16 1, i16 1, i16 1> }, align 32
...
call void @llvm.memcpy.p0i8.p0i8.i64(i8* %0, i8* bitcast (%class.simd* @vc to i8*), i64 32, i1 false)
which is not handled properly by the BE. W/o {{constexpr}}, the constant becomes an immediate of the primitive vector type, and memcpy is replaced with store. And this is handled by the BE correctly.
There was a problem hiding this comment.
I checked on a simplified sample - simd(const simd &other) = default will lead to a memcpy compared to __esimd_vstore (as Denis mentions above) done today, which may cause CG problems. Since keeping it non-default does not change compatibility, we can add =default later if we find a proper way. @rolandschulz, I'm approving and merging, if you still have concerns - we can discuss and fix as a separate PR as you suggest above.
There was a problem hiding this comment.
It seems like a good temporary solution. Note that it isn't trivial if is isn't =default. And by not being trivial it can't be used for those things limited to trivial types (e.g. bit_cast). In general it is also an ABI break to change whether a type is trivial because some ABIs CC is different for trivial vs non-trivial types. Not sure whether any of this is a concern here.
No description provided.