[libcxx-commits] [PATCH] D144362: [libcxx] Updated <experimental/simd> based on Parallelism-TS N4808

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

Thanks for working on this! Incidentally, I recently received a report that our simd implementation was broken in all sorts of way, and it's great to get it sorted out.



================
Comment at: libcxx/include/experimental/__simd/scalar.h:16
+
+_LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL_SIMD_ABI
+
----------------
philnik wrote:
> Mordante wrote:
> > philnik wrote:
> > > IMO it would be a lot nicer to just use
> > > ```lang=c++
> > > namespace std::experimental {
> > > inline namespace parallelism_v2 {
> > > 
> > > } // namespace parallelism_v2
> > > } // namespace std::experimental
> > > ```
> > > While it's marginally more verbose, it's a lot easier to read. (`namespace std::experimental::inline parallelism_v2` doesn't work because that's C++20)
> > > @ldionne @Mordante any thoughts here?
> > > 
> > > (Don't apply this before others have commented, this is a more general discussion)
> > I don't have a real preference. IIRC `_LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL_SIMD_ABI` is the more typical way in libc++.
> Yes, it's the more typical way, but AFAICT mostly for symmetry with `_LIBCPP_BEGIN_NAMESPACE_STD`. There it makes a lot more sense IMO, since that namespace actually changes depending on the configuration. That's not the case for experimental stuff. We also don't have `_LIBCPP_BEGIN_NAMESPACE_STD_RANGES/FORMAT/etc.`.
Is there any reason for not doing (formatted however you prefer):

```
_LIBCPP_BEGIN_NAMESPACE_STD
namespace experimental { inline namespace parallelism_v2 {
  ....
}}
_LIBCPP_END_NAMESPACE_STD
```

? Then we could get rid of `_LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL_SIMD_ABI` altogether.


================
Comment at: libcxx/include/experimental/simd:225
+            const simd<T, Abi>& hi);
 } // parallelism_v2
 } // std::experimental
----------------
I would like to guard this whole header under `_LIBCPP_ENABLE_EXPERIMENTAL`. Users should use `-fexperimental-library` to be able to use the TSes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144362