[libcxx-commits] [PATCH] D144363: [libcxx] <experimental/simd> Added simd width functions, simd_size traits and related tests

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

This looks pretty good now. I have a  few comments, but nothing major.



================
Comment at: libcxx/docs/Status/ParallelismProjects.csv:11
 | `[parallel.simd.traits] <https://wg21.link/N4808>`_, "simd type traits is_simd_flag_type", None, Yin Zhang, |In Progress|
-| `[parallel.simd.traits] <https://wg21.link/N4808>`_, "simd type traits simd_size", None, Yin Zhang, |In Progress|
+| `[parallel.simd.traits] <https://wg21.link/N4808>`_, "`simd type traits simd_size[_v] <https://reviews.llvm.org/D144363>`_", [parallel.simd.traits] is_abi_tag[_v], Yin Zhang, |Complete|
 | `[parallel.simd.traits] <https://wg21.link/N4808>`_, "simd type traits memory_alignment", None, Yin Zhang, |In Progress|
----------------
Joy12138 wrote:
> philnik wrote:
> > This looks wrong. Should there have been two lines?
> > This looks wrong. Should there have been two lines?
> 
> This line corresponds to traits `simd_size` and `simd_size_v`.
> 
> And traits `is_abi_tag_v` is the dependency because the implementation of `simd_size_v` used it for SFINAE check. Am I right?
Ahh, I missed that `is_abi_tag` is a dependency. Never mind then.


================
Comment at: libcxx/docs/Status/ParallelismProjects.csv:17
 | `[parallel.simd.class] <https://wg21.link/N4808>`_, "`Class template simd declaration and alias <https://reviews.llvm.org/D144362>`_", [parallel.simd.abi], Yin Zhang, |Complete|
+| `[parallel.simd.class] <https://wg21.link/N4808>`_, "`Class template simd width function <https://reviews.llvm.org/D144363>`_", [parallel.simd.traits] simd_size[_v], Yin Zhang, |Complete|
 | `[parallel.simd.class] <https://wg21.link/N4808>`_, "Class template simd implementation", None, Yin Zhang, |In Progress|
----------------
Joy12138 wrote:
> philnik wrote:
> > I'm a bit confused by this entry and the one below. What exactly are you trying to mark as completed?
> > I'm a bit confused by this entry and the one below. What exactly are you trying to mark as completed?
> 
> This line and the one below correspond to the function `size()` in class `simd` and class `simd_mask`. And the function `size()` in class `simd` is dependent on traits `simd_size_v`. The function `size()` in class `simd_mask` is dependent on the function `size()` in class `simd`.
Ok. I think it would be clearer to use `simd<>::size()` and `simd_mask<>::size()`instead.


================
Comment at: libcxx/include/experimental/__simd/simd.h:27
+public:
+  using value_type = _Tp;
+  using mask_type  = simd_mask<_Tp, _Abi>;
----------------
These aliases need tests too. Just check for a few instantiations that they exist and have the expected types.


================
Comment at: libcxx/include/experimental/__simd/utility.h:10
 
-#ifndef _LIBCPP_EXPERIMENTAL___SIMD_SIMD_H
-#define _LIBCPP_EXPERIMENTAL___SIMD_SIMD_H
+#ifndef _LIBCPP_EXPERIMENTAL___SIMD_UTILITY_H
+#define _LIBCPP_EXPERIMENTAL___SIMD_UTILITY_H
----------------
Joy12138 wrote:
> philnik wrote:
> > `utility` doesn't really seem like that nice of a name for a header. Do you have plans to add more functions? If yes, which ones?
> Yes, this file is mainly used to implement some internal functions that are not easy to classify. You can see them here: https://reviews.llvm.org/D139421#change-93rWXSd8B8Z9
Hmm, I can't come up with a good name for them right now. No action required right now.


================
Comment at: libcxx/include/experimental/__simd/utility.h:20
 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>>;
-
+_LIBCPP_HIDE_FROM_ABI constexpr bool __is_vectorizable() {
+  return is_arithmetic_v<_Tp> && !is_const_v<_Tp> && !is_volatile_v<_Tp> && !is_same_v<_Tp, bool>;
----------------
Joy12138 wrote:
> philnik wrote:
> > Why isn't this simply a variable template?
> This is mainly used to give a flag for SFINAE check. I think it's better to use it as a function. WDYT?
Since this is a type trait, a variable template is the more canonical way to write it. It also avoids evaluating the same trait multiple times.


================
Comment at: libcxx/test/libcxx/transitive_includes/cxx03.csv:265
 experimental/simd cstddef
+experimental/simd type_traits
 experimental/string experimental/memory_resource
----------------
These shouldn't be required anymore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144363