[libcxx-commits] [PATCH] D144362: [libcxx] <experimental/simd> Add ABI tags, class template simd/simd_mask implementations. Add related simd traits and tests.

Tue Apr 18 07:38:37 PDT 2023

philnik added a comment.

In D144362#4277646 <https://reviews.llvm.org/D144362#4277646>, @Joy12138 wrote:

> @philnik Hi! I have resolved most of the comments and marked them as done. Comments that have not yet been marked may require further discussion.
>
> Do you have any other comments? If not, I will continue to organize and submit the remaining content based on this patch. That may also contribute to the discussion of the overall framework.
>
> By the way, does CI need to be fixed? CI errors do not seem to be introduced by this patch.

The CI problems are unrelated.

================
Comment at: libcxx/include/experimental/__simd/declarations.h:15-23
+_LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL
+inline namespace parallelism_v2 {
+template <class _Tp, class _Abi>
+class simd;
+template <class _Tp, class _Abi>
+class simd_mask;
+} // namespace parallelism_v2
----------------
There can be a few more newlines throughout.

================
Comment at: libcxx/include/experimental/simd:74
+    template<class _Tp> inline constexpr int max_fixed_size = 32;
+    template<class _Tp> using compatible = __vec_ext<16 / sizeof(_Tp)>;
+    template<class _Tp> using native = __vec_ext<_LIBCPP_NATIVE_SIMD_WIDTH_IN_BYTES / sizeof(_Tp)>;
----------------
Joy12138 wrote:
> philnik wrote:
> > This seems like something that might change from platform to platform. Could you add a `TODO: make this platform dependent`?
> > This seems like something that might change from platform to platform. Could you add a `TODO: make this platform dependent`?
> 
> These tags will not be platform dependent until the third layer work is started.
Then I'd like a TODO for this, just in case someone else continues the work.

================
Comment at: libcxx/include/experimental/simd:85-93
+  // class template simd [simd.class]
+  template<class _Tp, class _Abi = simd_abi::compatible<_Tp>> class simd;
+  template<class _Tp> using native_simd = simd<_Tp, simd_abi::native<_Tp>>;
+  template<class _Tp, int _Np> using fixed_size_simd = simd<_Tp, simd_abi::fixed_size<_Np>>;
+
+  // class template simd_mask [simd.mask.class]
+  template<class _Tp, class _Abi = simd_abi::compatible<_Tp>> class simd_mask;
----------------
Joy12138 wrote:
> Joy12138 wrote:
> > philnik wrote:
> > > These should probably get their own headers. I guess the classes are not super small.
> > In our design of the SIMD library structure, these specializations of the `simd/simd_mask` will not have their own headers. This is because the implementations are not divided by the ABI tags of the user interface layer, but by the internal ABI tags. These internal tags do not correspond one-to-one with the user interface layer tags.
> > 
> > Let me briefly introduce the current structure design:
> > 
> > We divide the structure into three layers, with the specific implementations in the second and third layers. Each layer uses the ABI tags corresponding to that layer for template specialization.
> > 
> > The first layer is the external user interface layer, which corresponds to the external ABI tags given in the specification, such as `scalar`, `native`, `compatible`, and `fixed_size`.
> > 
> > The second layer is the common internal implementation layer, which is platform-independent. This layer corresponds to the internal ABI tags `__scalar` and `__vec_ext`. The implementation of the `__vec_ext` ABI tag uses Clang vector extensions (GCC builtins) as the storage type and Clang vector operators and built-ins to implement vector operations. We have tested this on various platforms and confirmed that the implementation of `__vec_ext` can correctly generate SIMD instructions supported by the current platform. We believe this will be a common solution for all platforms and will also serve as a fallback when the platform does not support SIMD.
> > 
> > The third layer is the platform-dependent optimization layer. Since the common layer implementation can correctly generate SIMD instructions, we only need to use some unique platform features or instructions to optimize parts of the implementation with poor performance on the common layer. This work has not yet started, and we may use some platform-specific internal ABI tags such as `__AVX`, `__NEON`, etc. at that time.
> > 
> > Therefore,
> > 
> > These should probably get their own headers. I guess the classes are not super small.
> 
> These specializations has no direct corresponding implementation. In the current implementation, they all indirectly correspond to `__vec_ext`, but this may also change in the future due to changes of ABI tags. So it seems inappropriate to put them all in `vec_ext. h`.
> 
Moving them into separate headers is for ease of maintenance and avoiding transitively including too many symbols.

================
Comment at: libcxx/include/experimental/simd:96-99
+  template<class _Tp, class = void> struct __is_abi_tag_impl : false_type {};
+  template<class _Tp> struct __is_abi_tag_impl<_Tp, enable_if_t<_Tp::__is_abi_tag>> : bool_constant<_Tp::__is_abi_tag> {};
+  template<class _Tp> struct is_abi_tag : __is_abi_tag_impl<_Tp> {};
+  template<class _Tp> inline constexpr bool is_abi_tag_v = is_abi_tag<_Tp>::value;
----------------
Joy12138 wrote:
> philnik wrote:
> > I think it would be easier to move this to it's own header, and then specialize `is_abi_tag` per ABI tag, i.e.
> > ```lang=c++
> > // traits.h
> > template <class _Tp>
> > inline constexpr bool is_abi_tag_v = false;
> > 
> > template <class _Tp>
> > struct is_abi_tag : bool_constant<is_abi_tag_v<_Tp>> {};
> > 
> > // scalar.h
> > template <>
> > inline constexpr bool is_abi_tag_v<__scalar> = true;
> > 
> > // vec_ext.h
> > template <int _Np>
> > inline constexpr bool is_abi_tag_v<__vec_ext<_Np>> = true;
> > ```
> > Note that this also implements `is_abi_tag` in terms of `is_abi_tag_v` and not the other way around. Most people just use the latter, so we should avoid instantiating a class every time. This part also applies to `is_simd` and `is_simd_mask`.
> This looks great, but there are often references to other interfaces in the implementation of traits. If traits are to be implemented in different ABI tag headers, we may need to add some declarations to the outer layer of these headers. For example, class `simd`/`simd_mask` will be referenced in the `is_simd`/`is_simd_mask` implementation. Should I use a new header (`declaration. h`) to add these declarations?
You just specialize the traits in the header where the class is defined. No need to forward-declare anything.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144362/new/

https://reviews.llvm.org/D144362