Page MenuHomePhabricator

[libcxx] implement <experimental/simd> declarations based on P0214R7.

Authored by timshen on Dec 12 2017, 5:41 PM.



The patch includes all declarations, and also implements the following features:

  • ABI.
  • narrowing-conversion related SFIANE, including simd<> ctors and (static_)simd_cast.

Event Timeline

timshen created this revision.Dec 12 2017, 5:41 PM

I beg you to resubmit this patch relative to "libcxx"; I had to arc export this patch and patch -p1 < your.diff manually rather than just arc patch D41148.


Does it causes a problem if the logic of is_convertible is folded into __is_non_narrowing_convertible?


Really? decltype(simd(...), 0) = 0 seems passing your tests.

87 ↗(On Diff #126662)

No more return 0; in main; no other files do so.

37 ↗(On Diff #126662)

> > is not a fashion...

timshen updated this revision to Diff 126813.Dec 13 2017, 12:47 PM

Address comments:

  • Generator ctor is implementable
  • Format issues of tests
  • SFINAE on __is_non_narrowing_convertible for arithmetics only.
timshen marked 4 inline comments as done.Dec 13 2017, 12:48 PM
timshen added inline comments.

Avoided to answer this question by sfinae on is_arithmetic.

EricWF added inline comments.Dec 13 2017, 2:05 PM

Please add the license header similar to experimental/filesystem.


The first include should be <experimental/__config>


(1) _LIBCPP_STD_VER is never less than 11. It's an odd libc++ quirk.
(2) We don't normally generate explicit errors when headers are used incorrectly.


I would just remove this. The experimental part of the name should say enough.

Also we normally wait until an implementation is mostly complete before landing it.


Please define macros to open and close the namespace similar to _LIBCPP_BEGIN_NAMESPACE_EXPERIMENTAL_FILESYSTEM in <experimental/__config>

15 ↗(On Diff #126813)

Please create a directory called simd/traits/ and then put each trait in a separate test file under that directory.
The foo_v tests should be in the same file as the foo tests.

timshen updated this revision to Diff 126872.Dec 13 2017, 4:36 PM
timshen marked 7 inline comments as done.

Address second round of comments.

timshen added inline comments.Dec 13 2017, 4:36 PM

Also we normally wait until an implementation is mostly complete before landing it.

What's the definition of "landing"? I prefer to implement this library in multiple patches, does that mean I keep working on the next ones and finally commit all of them all at once? If that's the case, SGTM.

15 ↗(On Diff #126813)

Thanks! Done for all tests. Used directory names like "simd.cons", "simd.traits", "simd.casts".

timshen updated this revision to Diff 127399.Dec 18 2017, 11:38 AM

Formatted the files.

timshen updated this revision to Diff 127622.Dec 19 2017, 4:32 PM

include "test_macros.h" in the tests

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


I'm going to stop here, because all the things I've noted are ticky-tack; formatting and minor changes.
More substantial comments coming soon.


A blank line between the separate groups (as in the paper) will make this much easier to read


And here you have an extra line - go figure.


blank line, extra space (see below ) should be (see below)


This is better formatting than in the paper :-)


These would be much easier to read lined up, and with blank lines.

template <class Abi> floatv<Abi>   acos(floatv<Abi>   x);
template <class Abi> doublev<Abi>  acos(doublev<Abi>  x);
template <class Abi> ldoublev<Abi> acos(ldoublev<Abi> x);

template <class Abi> floatv<Abi>   asin(floatv<Abi>   x);
template <class Abi> doublev<Abi>  asin(doublev<Abi>  x);
template <class Abi> ldoublev<Abi> asin(ldoublev<Abi> x);

Do you need the trailing return type here? instead of something like decltype(_to{declval<_From>()})

Then you don't need to name a, and can get rid of the (gcc-specific) attribute.


This won't work if _Tp has an explicit default ctor (which is probably not true for anything in SIMD). Do you lose anything by writing return _Tp{} ?


Isn't the parallelism TS based on C++17?

timshen updated this revision to Diff 139393.Mar 21 2018, 4:42 PM
timshen marked 7 inline comments as done.

Address comments.

timshen added inline comments.Mar 21 2018, 4:42 PM

I intended to have a C++11 (and C++14) compatible implementation.

When you add a new header file to libc++, you have to update two additional files:

  • include/module.modulemap
  • test/libcxx/

See for an example of how to do this.

3 ↗(On Diff #139393)

Surely you didn't really mean to name this file "geneartor.pass.cpp"

mclow.lists added inline comments.Apr 4 2018, 3:54 PM

Needs negative tests.


Negative tests, too, please.


It's good that you check that all the things do pass, but you should have some failing tests, too:

static_assert(!is_simd_flag_type<void>::value, "");
static_assert(!is_simd_flag_type<int>::value, "");
static_assert(!is_simd_flag_type<float>::value, "");
static_assert(!is_simd_flag_type<std::string>::value, "");
static_assert(!is_simd_flag_type<fixed_size_simd<int8_t>>::value, "");
static_assert(!is_simd_flag_type<native_simd_mask<int8_t>>::value, "");

How about a couple more negative tests here?
You've got int, how about float, and std::string and a few of the simd types?

timshen updated this revision to Diff 141188.Apr 5 2018, 11:37 AM

Addressed comments.

timshen marked 5 inline comments as done.Apr 5 2018, 11:39 AM
timshen added inline comments.

I used "UserType" instead of std::string, as I don't want to pull in the whole <string> header. I figured that UserType should serve the same purpose.

timshen updated this revision to Diff 141436.Apr 6 2018, 3:09 PM
timshen marked an inline comment as done.

Update file comments copy-paste error.

mclow.lists accepted this revision.Apr 23 2018, 11:05 AM

This looks ready to land to me.


Minor formatting nit. When I have stacks of tests like this, I usually leave a space after the '(', so that positive and negative tests line up, and so I can see at a glance which are which.


static_assert( is_simd_mask_v<fixed_size_simd_mask<double, 32>>, "");
static_assert(!is_simd_mask_v<void>, "");
This revision is now accepted and ready to land.Apr 23 2018, 11:05 AM
timshen updated this revision to Diff 143610.Apr 23 2018, 11:49 AM

Update formatting on static_asserts.

This revision was automatically updated to reflect the committed changes.