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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

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

> @philnik @Mordante Thanks for your attention to this patch. I don't know how to reply to you all in the comment, so I wrote all replies here.
>
>> Thanks for working on this. Is N4808 voted in? I can only find a draft version.
>
> It seems it is still draft. Should I continue to refer to this document, or which other version?
>
>> Why do you remove all of the old implementation? Is it so radically different from what you want to implement?
>
>
>
>> I'm wondering the same. Normally we don't remove code, since it may break existing users.
>> However I think our current implementation is so minimal that risk isn't too great.
>
> Yes, although we referred to and followed the original implementation, we have made great adjustments to the overall implementation framework and test framework.
>
> Implementation framework:
>
> Now we use the internal ABI as a template parameter. And we use the template class `simd_storage/mask_storage` to define the storage type of the internal ABI, use the template class `simd_traits/mask_traits` to implement operations of the internal ABI. We put these template specializations of each internal ABI in a seperate header file. We believe that this will help to expand the internal ABI and to make target-specific optimizations in the future.
>
> Test framework:
>
> We also reconstructed the test code structure. Added a test framework to improve test coverage. Because most external user interfaces are related to class `simd/simd_mask`, we have traversed all possible data types and ABI tags combinations of class `simd/simd_mask` in related interface tests.
>
> We have now completed the implementation of a stable version (Refer to this revision: https://reviews.llvm.org/D139421). This stable version included all external user interfaces except the Math Library. And added corresponding external interfaces tests. I hope that the code finally submitted to the upstream will be closer to the current available implementation. So it may be hard to advance on the original implementation. I prefer to remove the original implementation and build our existing framework from the most basic part. I plan to submit the follow-up implementation step by step In the next two months to completely cover all interfaces. I suggest we can consider to land all these patches together after they are reviewed and approved to avoid breaking existing users.
>
>> The synopsis should be updated as we go, to show what has actually been implemented. I am aware that this isn't the case currently, so let's fix this.
>
>
>
>> Let's keep the stable names. Section numbers are just a nightmare to maintain and will be wrong. At least they are everywhere they are used currently.
>
>
>
>> +1 Especially since these numbers will change if this is adopted on the Standard. Using the stable names gives a higher probability these references remain valid.
>
>
>
>> Could you remove all the declaration-only stuff in a separate PR? We normally don't have the interface declared if it's not defined.
>
> Summary (correct me if I misunderstood):
>
> 1. No declarations.
> 2. Give synopsis of implemented interfaces only.
> 3. Keep original stable names of sections without section numbers.

Yes, that's essentially it. I would suggest a patch that just removes everything from the header and the tests and then have a stack that re-implements the already-implemented stuff. Then the stack can be landed together. I think it would make sense to not implementing anything new in this stack between removing the implementation and having everything that is already implemented again, so it's essentially just a bunch of refactoring. On top of that we can them implement more of the interface. Looking at our current implementation, there is not that much implemented, so I guess the stack shouldn't be that large.



================
Comment at: libcxx/include/experimental/__simd/scalar.h:16
+
+_LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL_SIMD_ABI
+
----------------
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.`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144362