Page MenuHomePhabricator

[libcxx] implement <experimental/simd> concat() and split()
Needs ReviewPublic

Authored by timshen on Dec 19 2017, 1:21 PM.

Details

Summary

This patch implements the extended version (see P0820) of P0214
concat() and split().

If compiled with Clang, sometimes they are implemented in terms of
__builtin_shufflevector to avoid relying on auto-vectorization.

Event Timeline

timshen created this revision.Dec 19 2017, 1:21 PM
timshen updated this revision to Diff 129009.Jan 8 2018, 4:58 PM

Rebased.

timshen edited the summary of this revision. (Show Details)Apr 5 2018, 11:45 AM
mclow.lists added inline comments.Jul 17 2018, 9:25 AM
libcxx/include/experimental/simd
972

P0820 has been adopted, no need for this comment any more.

1421

I see no change here other than fixing a typo - correct?

1563

In general, we try to keep all the compiler-specific bits in <__config>. They tend to grow/mutate over time, and so it's nice to have them all in one place.

Better to define something like _LIBCPP_HAS_BUILTIN_SHUFFLEVECTOR and use that; then if GCC gets religion and adds it, you will only have to update a single place. Also, it makes the reading easier - no more looking at this and wondering "Why are you requiring clang here?"

Is this the only place you plan on using __simd_shuffle? If so, why not a member function.

timshen updated this revision to Diff 158092.Jul 30 2018, 2:58 PM
timshen marked 2 inline comments as done.
timshen edited the summary of this revision. (Show Details)

Update based on comments.

A note on test cases: I only used simds ints to test split() and concat(), as both functions don't specialize on the element type, unlike the constructors.

libcxx/include/experimental/simd
1421

That is correct.

1563

GCC already got religion and had it, but in a radically different interface. There is little chance that they will merge the interface in the future.

The GCC one is called __builtin_shuffle (https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html). I haven't added it only because I don't care about the performance on GCC.

It's rather non-trivial to eliminate the difference between them, certainly more than a macro like _LIBCPP_HAS_BUILTIN_SHUFFLEVECTOR.

Do you have any other way to improve the readability?

grosser removed a subscriber: grosser.
grosser added a subscriber: grosser.

Hi @timshen,

I am very interested in these patches. Any chance you can take up the upstreaming process again?

Best,
Tobias

Hi @timshen,

I am very interested in these patches. Any chance you can take up the upstreaming process again?

I'm glad to spend time upstreaming these patches. Now we just need a libc++ maintainer to take on the reviews. I'll try to contact mclow, but I'm not sure how exactly to reach Marshall.

It seems this patch went through at least one review and the only open comment is the discussion if __builtin_shuffle should be placed in the configuration. From my perspective, both solutions are technically feasible. While it seems unlikely that gcc will gain this specific builtin, I can see @mclow.lists preferring to be consistent in keeping compiler-specific stuff in one file rather than having to evaluate for each extension the likeliness of this extension being adopted over time.

If you don't feel strongly about this change, I guess moving it to _config would address this last point and would require then only an ok, but no further discussions.

If you cannot reach @mclow.lists I am happy to either reach out or check for other reviewers. Eric Fiselier seems very active at the moment.

Tobias,

I spoke to @EricWF who is willing to take a look at all these patches. However, I don't know when exactly will the review starts.

Tobias,

I spoke to @EricWF who is willing to take a look at all these patches. However, I don't know when exactly will the review starts.

Amazing. I am super interested in seeing this moving but am happy to give you some time to get started. ;-) Thanks a lot for picking this up!

maxf added a subscriber: maxf.Jul 19 2019, 2:38 AM

@EricWF thanks again for offering your help here. We are not in a rush here, but I wonder if you happen to have a rough estimate when you might have the time to look into these patches?

@EricWF and @timshen, @maxf and I are indeed very interested in seeing this library upstreamed soon. Over the last weeks, we developed a software prototype based on these bindings and already started to tune performance on clang/LLVM. As part of this, we looked into efficient vector code generation, started to implement where expressions, ... Currently, everything is very much prototype quality, but we would very much prefer to get things integrated into libcxx. For this, it's important to have a working and committed implementation, such that we can start submitting patches ourselves and can obtain feedback on these patches early on. Otherwise, we might take directions that won't really be compatible with libcxx. How can we best help with getting these patches upstream?

cpplearner added a project: Restricted Project.Aug 17 2019, 1:04 PM