Page MenuHomePhabricator

[libcxx] <experimental/simd> Add ABI tags, class template simd/simd_mask implementations. Add related simd traits and tests.
AcceptedPublic

Authored by Joy12138 on Feb 19 2023, 11:02 PM.

Details

Reviewers
philnik
Group Reviewers
Restricted Project
Summary

[libcxx] <experimental/simd> Add ABI tags, class template simd/simd_mask implementations.
[libcxx] <experimental/simd> Add traits is_abi_tag[_v], is_simd[_v] and is_simd_mask[_v].
[libcxx] <experimental/simd> Add related tests.

Diff Detail

Unit TestsFailed

TimeTest
35,770 mslibcxx CI - Clang-cl (DLL) > llvm-libc++-shared-clangcl-cfg-in.libcxx::transitive_includes.sh.cpp
Script: -- : 'RUN: at line 71'; mkdir C:\ws\w3\llvm-project\libcxx-ci\build\clang-cl-dll\test\libcxx\Output\transitive_includes.sh.cpp.dir\t.tmp
37,860 mslibcxx CI - Clang-cl (no vcruntime exceptions) > llvm-libc++-shared-no-vcruntime-clangcl-cfg-in.libcxx::transitive_includes.sh.cpp
Script: -- : 'RUN: at line 71'; mkdir C:\ws\w6\llvm-project\libcxx-ci\build\clang-cl-no-vcruntime\test\libcxx\Output\transitive_includes.sh.cpp.dir\t.tmp
14,880 mslibcxx CI - No experimental features > llvm-libc++-shared-cfg-in.libcxx::transitive_includes.sh.cpp
Script: -- : 'RUN: at line 71'; mkdir /home/libcxx-builder/.buildkite-agent/builds/google-libcxx-builder-2f9cef8fbdb5-1/llvm-project/libcxx-ci/build/generic-no-experimental/test/libcxx/Output/transitive_includes.sh.cpp.dir/t.tmp

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Joy12138 updated this revision to Diff 507248.Mar 21 2023, 11:36 PM

Fix rebase error

Joy12138 updated this revision to Diff 511272.Apr 5 2023, 10:19 PM

Update test structure with type_algorithms.h

Joy12138 marked an inline comment as done.Apr 5 2023, 10:20 PM
Joy12138 updated this revision to Diff 511294.Apr 5 2023, 11:57 PM

try cxx03

Joy12138 updated this revision to Diff 511338.Apr 6 2023, 2:33 AM

Move traits to separate headers.

Joy12138 marked an inline comment as done.Apr 6 2023, 2:35 AM
Joy12138 updated this revision to Diff 511340.Apr 6 2023, 2:53 AM

Reorder includes

Joy12138 updated this revision to Diff 513524.Apr 14 2023, 3:46 AM

Fix C++17

@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.

@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.

libcxx/include/experimental/__simd/declarations.h
15–23 ↗(On Diff #513524)

There can be a few more newlines throughout.

libcxx/include/experimental/simd
74

Then I'd like a TODO for this, just in case someone else continues the work.

85–93

Moving them into separate headers is for ease of maintenance and avoiding transitively including too many symbols.

96–99

You just specialize the traits in the header where the class is defined. No need to forward-declare anything.

Joy12138 added inline comments.Apr 18 2023, 10:38 PM
libcxx/include/experimental/simd
96–99

Sorry, my previous description was not clear enough. The main issue is that in our design, for more code reuse and more flexible use of internal interfaces, the structure of the external interface layer and internal implementation layer is not consistent. And template specialization does not directly occur on external interfaces such as simd/simd_mask classes.

We have designed four modules, SIMD/MASK storage type and SIMD/MASK operations, for different ABI tags at the implementation layer, which will be used by various external interfaces, including simd/simd_mask, where_expression and non-member functions.

i.e.

// simd
template <class _Tp, class _Abi>
class simd {
  using _Impl    = __simd_operations<_Tp, _Abi>;
  using _Storage = __simd_storage<_Tp, _Abi>;
  
  _Storage __s_;
  
  simd operator+(const simd& __lhs, const simd& __rhs) {
    return _Impl::__plus(__lhs.__s_, __rhs.__s_);
  }
}

// scalar.h
template <class _Tp>
struct __simd_storage<_Tp, simd_abi::__scalar> {
	......
};

template <class _Tp>
struct __simd_operations<_Tp, simd_abi::__scalar> {
	......
};

// vec_ext.h
template <class _Tp, int _Np>
struct __simd_storage<_Tp, simd_abi::__vec_ext<_Np>> {
	......
};

template <class _Tp, int _Np>
struct __simd_operations<_Tp, simd_abi::__vec_ext<_Np>> {
	......
};

Since the specialization does not occur on external interfaces, the external interfaces is now directly placed in the simd header. if the implementation of simd traits is to be placed in a separate header, a forward declaration is required, and if it is placed together with the external interfaces, it will also be placed in the simd header.

Do you have any better suggestions for the current code structure?

Joy12138 added inline comments.Apr 18 2023, 10:47 PM
libcxx/include/experimental/simd
96–99

In addition, most simd traits are platform independent, is it not necessary to specialize for all traits?

Joy12138 updated this revision to Diff 515604.Apr 20 2023, 11:47 PM

Add newlines and TODOs.

Joy12138 marked 4 inline comments as done.Apr 21 2023, 12:31 AM
This comment was removed by Joy12138.

@philnik Just ping for the above discussion.

philnik added inline comments.Apr 30 2023, 4:19 PM
libcxx/include/experimental/simd
96–99

I'm a bit confused what exactly you are talking about. Was this meant for a different thread?

Joy12138 added inline comments.May 3 2023, 6:56 PM
libcxx/include/experimental/simd
96–99

I'm sorry my description confused you. Maybe I shouldn't have put this content in this thread. I just want to discuss the overall structure and the use of template specialization. And they have some impact on the implementation of simd traits.

What I mainly want to express is that since we do not directly specialize templates on external user interfaces, there will not be directly definitions and implementations of such as
simd<_Tp, __vec_ext<_Np>>. It is basically composed of simd_storage<_Tp, __vec_ext<_Np>> and simd_operations<_Tp, __vec_ext<_Np>>. So we cannot directly define is_simd<simd<_Tp, __vec_ext<_Np>>> without forward-declare of simd class.

777ki added a subscriber: 777ki.Tue, May 16, 9:08 PM
philnik accepted this revision.Sat, May 20, 1:36 PM

LGTM % nits. (and sorry for being so slow in reviewing this)

libcxx/test/std/experimental/simd/simd.traits/is_abi_tag.pass.cpp
56
57

Couldn't the static_asserts be inlined here? The CheckIsAbiTag* could simply be tag types.

libcxx/test/std/experimental/simd/test_utils.h
36

This doesn't really seem with it given that it already just ex.

This revision is now accepted and ready to land.Sat, May 20, 1:36 PM
Joy12138 marked 7 inline comments as done.Mon, May 29, 11:09 PM

@philnik Hi! We rewrite the tests. Please review it again. Thank you!

philnik accepted this revision.Tue, May 30, 3:15 PM

The updated tests look really nice! Thanks!

You probably have to rebase on top of main to make the CI happy.

libcxx/test/std/experimental/simd/simd.traits/is_abi_tag.pass.cpp
31–39

clang-format will be happy with this.

libcxx/test/std/experimental/simd/test_utils.h
21–22

This is just integer_types, right?

Joy12138 updated this revision to Diff 527721.Thu, Jun 1, 9:10 PM

Fix test format

Joy12138 marked an inline comment as done.Thu, Jun 1, 9:21 PM
Joy12138 added inline comments.
libcxx/test/std/experimental/simd/test_utils.h
21–22

No, it is not the same.

The first template parameter of the simd class does not accept input of bool type. And the vector type corresponding to bool type is defined by simd_mask class.

In "type_algorithms.h", integer_types has the following definition:

// libcxx/test/support/type_algorithms.h:98
using integral_types = concatenate_t<character_types, signed_integer_types, unsigned_integer_types, type_list<bool> >;

It contains type bool. And we need arithmetic types without bool to test class simd.

I encountered a problem in subsequent development that currently seems to need to be resolved in this patch.

In the previous comment, you suggested that I place all simd traits in a separate header file traits.h. During the process of adding the rest traits implementations, I found that most of them require the use of other interfaces placed in the simd header, similar to is_simd requires the use of simd classes. Therefore, I added declaration.h as forward declarations. However, there are too many interfaces that need to be referenced in the future, which is not suitable to add forward declarations for all.

I think there are two solutions.
One is to keep all external user interfaces under the simd header, just like our previous implementation.
Another is to categorize all external user interfaces into separate header files. No implementations in the simd header and only including other header files in order. Then we may need to add traits.h, simd.h, simd_mask.h, aligned_tag.h, abi_tag.h, simd_casts.h, where_expression.h, etc.
Which one do you prefer?

I encountered a problem in subsequent development that currently seems to need to be resolved in this patch.

In the previous comment, you suggested that I place all simd traits in a separate header file traits.h. During the process of adding the rest traits implementations, I found that most of them require the use of other interfaces placed in the simd header, similar to is_simd requires the use of simd classes. Therefore, I added declaration.h as forward declarations. However, there are too many interfaces that need to be referenced in the future, which is not suitable to add forward declarations for all.

I think there are two solutions.
One is to keep all external user interfaces under the simd header, just like our previous implementation.
Another is to categorize all external user interfaces into separate header files. No implementations in the simd header and only including other header files in order. Then we may need to add traits.h, simd.h, simd_mask.h, aligned_tag.h, abi_tag.h, simd_casts.h, where_expression.h, etc.
Which one do you prefer?

I think it's easier to gollow the code when splitting up the headers, so I'd go for option 2.

libcxx/test/std/experimental/simd/test_utils.h
21–22

You show the implementation of integral_types, not integer_types ;-)

Joy12138 updated this revision to Diff 528756.Tue, Jun 6, 2:40 AM

Rebase, and reorganize file structure

Joy12138 updated this revision to Diff 528759.Tue, Jun 6, 2:46 AM

Use integer_types in test_utils.h

Joy12138 marked 2 inline comments as done.Tue, Jun 6, 2:48 AM
Joy12138 updated this revision to Diff 528768.Tue, Jun 6, 3:12 AM

Fix clang format

Joy12138 updated this revision to Diff 528771.Tue, Jun 6, 3:32 AM

Fix transitive includes in cxx26

Joy12138 updated this revision to Diff 528772.Tue, Jun 6, 3:36 AM

Fix order in CMakeList

Joy12138 updated this revision to Diff 528778.Tue, Jun 6, 4:09 AM

Fix transitive includes in cxx03 and cxx11

Joy12138 updated this revision to Diff 528806.Tue, Jun 6, 5:25 AM

Fix transitive includes in cxx14

Joy12138 updated this revision to Diff 529149.Tue, Jun 6, 9:17 PM

Fix transitive includes in cxx23

@philnik Hi! Could you please help me check the CI errors?

Last time it prompted me to remove the following statement in transitive includes of cxx23 and cxx26.

*** 174,179 ****

--- 174,180 ----

  experimental/regexregex

  experimental/setexperimental/memory_resource

  experimental/setset

- experimental/simdcstddef

  experimental/stringexperimental/memory_resource

  experimental/stringstring

  experimental/type_traitsinitializer_list



error: command failed with exit status: 1

But this time after I removed, it prompted me to add.

*** 174,179 ****

--- 174,180 ----

  experimental/regexregex

  experimental/setexperimental/memory_resource

  experimental/setset

+ experimental/simdcstddef

  experimental/stringexperimental/memory_resource

  experimental/stringstring

  experimental/type_traitsinitializer_list



error: command failed with exit status: 1

@philnik Hi! Could you please help me check the CI errors?

Last time it prompted me to remove the following statement in transitive includes of cxx23 and cxx26.

*** 174,179 ****

--- 174,180 ----

  experimental/regexregex

  experimental/setexperimental/memory_resource

  experimental/setset

- experimental/simdcstddef

  experimental/stringexperimental/memory_resource

  experimental/stringstring

  experimental/type_traitsinitializer_list



error: command failed with exit status: 1

But this time after I removed, it prompted me to add.

*** 174,179 ****

--- 174,180 ----

  experimental/regexregex

  experimental/setexperimental/memory_resource

  experimental/setset

+ experimental/simdcstddef

  experimental/stringexperimental/memory_resource

  experimental/stringstring

  experimental/type_traitsinitializer_list



error: command failed with exit status: 1

It looks to me like adding it is the correct thing. Do you know which bot didn't like the added line?

Joy12138 updated this revision to Diff 529497.Wed, Jun 7, 9:19 PM

Add transitive includes in cxx23

@philnik I added it again, you can see the current CI errors.

philnik added inline comments.Thu, Jun 8, 8:26 AM
libcxx/include/experimental/simd
61

This check should be done after including the detail headers.