This is an archive of the discontinued LLVM Phabricator instance.

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

Diff Detail

Repository
rL LLVM

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
1069 ↗(On Diff #126662)

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

1088 ↗(On Diff #126662)

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
1069 ↗(On Diff #126662)

Avoided to answer this question by sfinae on is_arithmetic.

EricWF added inline comments.Dec 13 2017, 2:05 PM
libcxx/include/experimental/simd
1 ↗(On Diff #126813)

Please add the license header similar to experimental/filesystem.

581 ↗(On Diff #126813)

The first include should be <experimental/__config>

590 ↗(On Diff #126813)

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

594 ↗(On Diff #126813)

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.

596 ↗(On Diff #126813)

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
594 ↗(On Diff #126813)

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
40 ↗(On Diff #139036)

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

67 ↗(On Diff #139036)

And here you have an extra line - go figure.

113 ↗(On Diff #139036)

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

144 ↗(On Diff #139036)

This is better formatting than in the paper :-)

316 ↗(On Diff #139036)

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);
626 ↗(On Diff #139036)

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.

653 ↗(On Diff #139036)

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

669 ↗(On Diff #139036)

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
669 ↗(On Diff #139036)

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
44 ↗(On Diff #139393)

Needs negative tests.

libcxx/test/std/experimental/simd/simd.traits/is_simd.pass.cpp
68 ↗(On Diff #139393)

Negative tests, too, please.

libcxx/test/std/experimental/simd/simd.traits/is_simd_flag_type.pass.cpp
27 ↗(On Diff #139393)

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
68 ↗(On Diff #139393)

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
68 ↗(On Diff #139393)

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 ↗(On Diff #141436)

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.