This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Implement <simd> ABI for Clang/GCC vector extension, constructors, copy_from and copy_to.
ClosedPublic

Authored by timshen on Dec 18 2017, 6:10 PM.

Details

Summary

This patch adds a new macro _LIBCPP_HAS_NO_VECTOR_EXTENSION for detecting
whether a vector extension (\_\_attribute\_\_((vector_size(num_bytes)))) is
available.

On the top of that, this patch implements the following API:

  • all constructors
  • operator[]
  • copy_from
  • copy_to

It also defines simd_abi::native to use vector extension, if available.
In GCC and Clang, certain values with vector extension are passed by registers,
instead of memory.

Based on D41148.

Diff Detail

Repository
rL LLVM

Event Timeline

timshen created this revision.Dec 18 2017, 6:10 PM
timshen updated this revision to Diff 127453.Dec 18 2017, 6:12 PM

Update description.

timshen edited the summary of this revision. (Show Details)Dec 18 2017, 6:13 PM
timshen updated this revision to Diff 127455.Dec 18 2017, 6:24 PM

Fix reference's access control.

timshen updated this revision to Diff 127458.Dec 18 2017, 6:36 PM

Fix return type of reference::opreator--.

timshen updated this revision to Diff 127638.Dec 19 2017, 5:48 PM

s/_LIBCPP_HAS_VECTOR_EXTENSION/_LIBCPP_HAS_NO_VECTOR_EXTENSION/

timshen edited the summary of this revision. (Show Details)Dec 19 2017, 5:48 PM
timshen updated this revision to Diff 128680.Jan 4 2018, 4:49 PM

Rebase. Also modify the simd reference implementation to separate the stored type and value_type (which could be bool later).

timshen updated this revision to Diff 129008.Jan 8 2018, 4:57 PM

Rebased.

timshen updated this revision to Diff 148062.May 22 2018, 11:38 AM

Chatted with Marshall a bit, we thought that it's bad for toolchain portability to support a C++17 library in C++11, especially with modifications w.r.t std::plus<>.

Remove the backport of std::integer_sequence to C++11.

timshen edited the summary of this revision. (Show Details)May 22 2018, 11:38 AM
miyuki added a subscriber: miyuki.Jul 4 2018, 7:32 AM

Looks good; a bunch of minor things.
Remember to target c++17.
You need to indulge your OCD when writing the tests.

libcxx/include/__config
1329 ↗(On Diff #148062)

We ususally organize these into "chunks", one per compiler.
The "Clang chunk" starts on 159, and goes to 483.
The "GCC chunk" starts on 484 and goes to 575.

It would match the organization better if you put this line:

#define _LIBCPP_HAS_NO_VECTOR_EXTENSION

in the file twice - once in the Clang chunk and once in the GCC chunk.

See _LIBCPP_HAS_UNIQUE_OBJECT_REPRESENTATIONS for an example of this.

libcxx/include/experimental/simd
726 ↗(On Diff #148062)

When we define user-visible macros in libc++, we prepend them with _LIBCPP_ to avoid collisions.
Accordingly, _SPECIALIZE_VEC_EXT should be named _LIBCPP_SPECIALIZE_VEC_EXT

936 ↗(On Diff #148062)

We got rid of this for N4755 - but I think you're fixing this in D44663.

964 ↗(On Diff #148062)

I have a weak preference for putting entire statements inside #ifdef blocks where possible.

1341 ↗(On Diff #148062)

Is this TODO still necessary?

libcxx/test/std/experimental/simd/simd.cons/broadcast.pass.cpp
79 ↗(On Diff #148062)

Again, do we care about other types than native_simd here?

libcxx/test/std/experimental/simd/simd.cons/default.pass.cpp
21 ↗(On Diff #148062)

Do we need any other ctors tested here? fixed_size_simd<int32_t, 4> for example?
Are there any post-conditions on the object created?

calling size() for example?

libcxx/test/std/experimental/simd/simd.mem/load.pass.cpp
22 ↗(On Diff #148062)

I am unsure about the desirability of testing this by hoisting it all into the global namespace.

for the pmr stuff, we did:

namespace ex = std::experimental::pmr;

and then referred to things as ex::XXX. That way, you don't have to do all the typing, but we don't hide any ADL issues.

Obviously, this applies to all the tests.

libcxx/test/std/experimental/simd/simd.mem/store.pass.cpp
10 ↗(On Diff #148062)

more unsupported :-)

51 ↗(On Diff #148062)

Don't need this anymore.

timshen updated this revision to Diff 157751.Jul 27 2018, 1:19 PM
timshen marked 6 inline comments as done.

Update based on the comments.

libcxx/include/experimental/simd
726 ↗(On Diff #148062)

Well _SPECIALIZE_VEC_EXT and _SPECIALIZE_VEC_EXT_32 are not user-visible, as they are (1) '#undef'ed after all uses, and (2) they start with underscore, so don't collide with existing user-defined macros.

1341 ↗(On Diff #148062)

I think so, as some operations are still not implemented, for example operator++().

libcxx/test/std/experimental/simd/simd.cons/default.pass.cpp
21 ↗(On Diff #148062)

Yes. Added more.

The test space is large and it's hard to test everything. I went "every line of the implementation should be covered" standard with myself, and only had native_simd's default ctor. fixed_size_simd<>'s ctor is exactly implemented by the same line, so I didn't bother.

But now the attention is already raised, it doesn't hurt to add more tests.

21 ↗(On Diff #148062)

Yes, size() is the post-condition. There is not much beyond that - the elements are not even value initialized.

I'm not going to rebase all the succeeding patches immediately onto this one, as it is painful and spamming emails. Rather, I'll only rebase the next patch in the line. So if you review more than one patch ahead (as you already did), you may see some stale patch context.

timshen added inline comments.Jul 27 2018, 1:23 PM
libcxx/include/experimental/simd
1341 ↗(On Diff #148062)

For test coverage I turned many of the tests into templates, in the hope not to duplicate the text and increase the coverage. I'm not sure if you like the idea.

Getting really close.

libcxx/include/experimental/simd
703 ↗(On Diff #157751)

Can these (__get and __set) be noexcept? Obviously, it depends on _Tp.

811 ↗(On Diff #157751)

If _Vp and _Tp have to name the same type, why have two of them?
What is the difference, and when would you use each one internally?

__simd_reference(__simd_storage<_Tp, _Abi>* __ptr, size_t __index);

vs.

__simd_reference operator=(_Vp __value) &&;
726 ↗(On Diff #148062)

I agree, but starting them with _LIBCPP means that people (like me) know that they come from libc++, and not (say) clang, or whatever C library we're sitting on top of.

timshen updated this revision to Diff 158038.Jul 30 2018, 12:08 PM
timshen marked 2 inline comments as done.

Update based on comments.

timshen added inline comments.Jul 30 2018, 12:09 PM
libcxx/include/experimental/simd
703 ↗(On Diff #157751)

Currently _Tp is always a subset of arithmetic type, so it's safe to say noexcept. Added them, as they may help with -O0 performance.

811 ↗(On Diff #157751)

If _Vp and _Tp have to name the same type, why have two of them?

Currently there is no difference. It's going to be different when simd_mask is implemented (_Vp can be bool, but _Tp may not be).

What is the difference, and when would you use each one internally?

This is not an internal class. This is part of the interface. IIRC since P0214R9 opreator=() is changed to something. In a later patch operator=() should be implemented as R9 specified.

mclow.lists accepted this revision.Jul 30 2018, 12:48 PM

LGTM. Thanks!

This revision is now accepted and ready to land.Jul 30 2018, 12:48 PM
This revision was automatically updated to reflect the committed changes.