Page MenuHomePhabricator

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

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

Details

Summary

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.

libcxx/include/experimental/simd
1070

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

1089

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

libcxx/test/std/experimental/simd/simd_cast.pass.cpp
87 ↗(On Diff #126662)

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

libcxx/test/std/experimental/simd/traits.pass.cpp
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.
libcxx/include/experimental/simd
1070

Avoided to answer this question by sfinae on is_arithmetic.

EricWF added inline comments.Dec 13 2017, 2:05 PM
libcxx/include/experimental/simd
2

Please add the license header similar to experimental/filesystem.

582

The first include should be <experimental/__config>

591

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

595

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.

597

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

libcxx/test/std/experimental/simd/traits.pass.cpp
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
libcxx/include/experimental/simd
595

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.

libcxx/test/std/experimental/simd/traits.pass.cpp
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

Rebased.

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.

libcxx/include/experimental/simd
41

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

68

And here you have an extra line - go figure.

114

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

145

This is better formatting than in the paper :-)

317

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);
627

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.

654

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{} ?

670

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
libcxx/include/experimental/simd
670

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/double_include.sh.cpp

See http://llvm.org/viewvc/llvm-project?view=revision&revision=329144 for an example of how to do this.

libcxx/test/std/experimental/simd/simd.cons/geneartor.pass.cpp
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
libcxx/test/std/experimental/simd/simd.traits/is_abi_tag.pass.cpp
45

Needs negative tests.

libcxx/test/std/experimental/simd/simd.traits/is_simd.pass.cpp
69

Negative tests, too, please.

libcxx/test/std/experimental/simd/simd.traits/is_simd_flag_type.pass.cpp
28

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, "");
libcxx/test/std/experimental/simd/simd.traits/is_simd_mask.pass.cpp
69

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.
libcxx/test/std/experimental/simd/simd.traits/is_simd_mask.pass.cpp
69

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.

libcxx/test/std/experimental/simd/simd.traits/is_simd_mask.pass.cpp
122

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.

Example:

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.