This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] <experimental/simd> Added aliagned flag types, traits is_simd_flag_type[_v], memory_alignment[_v] and related tests
ClosedPublic

Authored by Joy12138 on Jun 19 2023, 10:57 PM.

Details

Diff Detail

Event Timeline

Joy12138 created this revision.Jun 19 2023, 10:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2023, 10:57 PM
Herald added a subscriber: miyuki. · View Herald Transcript
Joy12138 requested review of this revision.Jun 19 2023, 10:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2023, 10:57 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Joy12138 updated this revision to Diff 532815.Jun 20 2023, 1:05 AM

Fix macro _LIBCPP_HIDE_FROM_ABI

Joy12138 updated this revision to Diff 532824.Jun 20 2023, 1:55 AM

Fix clang-tidy

philnik added inline comments.
libcxx/include/CMakeLists.txt
882

Looks like you forgot to update the status page.

libcxx/include/experimental/__simd/aligned_tag.h
36

Let's add __assume_aligned in __memory/assume_aligned.h to avoid using builtins randomly in the code. This lets us also assert if the alignment assumption isn't met (Please add a test for that!).

libcxx/include/experimental/__simd/traits.h
80

Same here - does this even work with variables and this needs a SFINAE test.

80

Isn't this implicit through is_simd_mask_v?

libcxx/include/experimental/__simd/utility.h
24–30

let's instead add a __bit_ceil for pre-C++20 usage.

libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp
26

Also the comment here - _LIBCPP_NATIVE_SIMD_WIDTH_IN_BYTES is libc++-specific, so it shouldn't be used in a stdtest.

libcxx/test/std/experimental/simd/test_utils.h
22

I don't think this is really an action item. The experimental implementation will probably be C++17 until we remove it. Only once it's (hopefully) landed in C++26 we can move the implementation and tests out of experimental and refactor any code.

23

Since you already mentioned that they are equivalent.

Joy12138 updated this revision to Diff 537013.Jul 4 2023, 2:49 AM
Joy12138 marked 5 inline comments as done.

Fix some comments

Joy12138 marked an inline comment as done.Jul 4 2023, 3:00 AM
Joy12138 added inline comments.
libcxx/include/experimental/__simd/traits.h
80

I don't think so. _Up should be able to be provided by the user. _Tp::value_type is just a default type.

Joy12138 marked an inline comment as done.Jul 4 2023, 3:02 AM
Joy12138 added inline comments.Jul 5 2023, 11:53 PM
libcxx/include/experimental/__simd/aligned_tag.h
36

If I understand correctly, what you want is a false test when provides incorrect alignment. Then both tests in this patch do not involve alignment inputs. Can we add these false tests in subsequent patches about constructors and load/store that involved with alignment inputs?

philnik added inline comments.Jul 6 2023, 12:20 AM
libcxx/include/experimental/__simd/aligned_tag.h
36

Oh yeah, sure. We can test it through the public interface.

libcxx/include/experimental/__simd/traits.h
80

Ah, I missed that _Up may be provided by the user. Never mind then.

libcxx/include/experimental/__simd/utility.h
22–39

Let's add these in their respective implementation detail headers. That way we avoid code duplication and can use them in other places in the codebase.

libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp
27

Again, to make sure the value is static.

29–30

It looks to me like this is technically implementation-defined. Am I correct in that interpretation?

39–40

I think this is technically also the case for float and double. It just happens to be the case that they are the IEEE types on about every platform.

libcxx/test/std/experimental/simd/test_utils.h
66

Newline!

Joy12138 added inline comments.Jul 10 2023, 1:30 AM
libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp
29–30

If my understanding of the document is correct, the basic type of the simd_mask is bool, while the storage type in the internal implementation layer is implementation-defined. Alignment should be calculated using bool.

Joy12138 updated this revision to Diff 539890.Jul 13 2023, 1:26 AM

Fix comments

Joy12138 marked 8 inline comments as done.Jul 13 2023, 1:30 AM
Joy12138 added inline comments.Jul 13 2023, 1:31 AM
libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp
39–40

is there any other modify required here?

Joy12138 updated this revision to Diff 539989.Jul 13 2023, 6:16 AM

Add SFINAE test for memory_alignment

Please rebase. It looks like the CI didn't run properly.

libcxx/docs/Status/ParallelismProjects.csv
7 ↗(On Diff #539989)

There doesn't seem to be a [parallel.simd.aligned] section. AFAICT this is just [parallel.simd.synopsis].

libcxx/include/__bit/bit_ceil.h
25 ↗(On Diff #539989)

Otherwise you're lying.

libcxx/include/__memory/assume_aligned.h
26 ↗(On Diff #539989)

This will break C++03 otherwise.

libcxx/include/experimental/__simd/aligned_tag.h
35
libcxx/include/experimental/__simd/traits.h
86

I think this might actually be wrong. The wording says that memory_alignment shouldn't contain value, not that the class should fail to instantiate.

libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp
29–30

I can't find any wording that suggests a required alignment. Consider a platform with a 32byte vector. If the platform can load that vector from a 16byte aligned address without any loss of performance, there is no reason to make memory_alignment return 32 instead of 16.

39–40

I guess technically yes, but I think it's fine as-is. So no.

Joy12138 updated this revision to Diff 540260.Jul 13 2023, 8:08 PM

Rebase and fix some comments

Joy12138 marked 5 inline comments as done.Jul 14 2023, 5:18 AM
Joy12138 added inline comments.
libcxx/include/experimental/__simd/traits.h
86

I'm a bit confuse about this. If no `class = enable_if_t<...>' here, there will be errors coming out. You can see the same situation in D144363#4497271 with 'simd_size'.

libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp
29–30

In that case, how should we modify this? Just add LIBCPP_STATIC_ASSERT temporarily?

philnik added inline comments.Jul 14 2023, 8:32 AM
libcxx/include/experimental/__simd/traits.h
86

I think we have to do something like

template <
    class _Tp,
    class _Up = typename _Tp::value_type,
    bool = (is_simd_v<_Tp> && __is_vectorizable_v<_Up>) || (is_simd_mask_v<_Tp> && is_same_v<_Up, bool>)>
struct memory_alignment : integral_constant<size_t, memory_alignment_v<_Tp, _Up>> {};

template <class _Tp, class _Up>
struct memory_alignment<Tp, _Up, false> {};

And yeah, this also applies to simd_size. The SFINAE tests you added are correct in that they ensure that there is no value member. I think the enable_if and test for simd_size_v can be removed, since they aren't required.

Sorry for not spotting this earlier.

Joy12138 updated this revision to Diff 540628.Jul 14 2023, 8:28 PM

Fix memory_alignment

philnik accepted this revision.Jul 14 2023, 10:06 PM

Thanks! LGTM with comments addressed.

libcxx/include/__bit/bit_ceil.h
31 ↗(On Diff #540628)

I think it should be fine to just remove the _LIBCPP_CONSTEXPR_SINCE_CXX14. It's not like it makes much of a difference in compile times.

libcxx/test/std/experimental/simd/simd.traits/memory_alignment.pass.cpp
2

I think you're missing a test that you can instantiate memory_alignment with an invalid type.

29–30

Yeah, I thin a LIBCPP_STATIC_ASSERT is the right call here.

This revision is now accepted and ready to land.Jul 14 2023, 10:06 PM
Joy12138 updated this revision to Diff 540846.Jul 16 2023, 7:24 PM

Fix some comments and CI errors

Joy12138 updated this revision to Diff 540856.Jul 16 2023, 10:01 PM

Fix transitive includes

Joy12138 marked 5 inline comments as done.Jul 16 2023, 10:34 PM
Joy12138 updated this revision to Diff 540936.Jul 17 2023, 3:48 AM

Fix memory_alignment_v

Joy12138 updated this revision to Diff 540939.Jul 17 2023, 3:52 AM

Add newline at end of file

Joy12138 marked an inline comment as done.Jul 17 2023, 3:56 AM

@philnik Could you please help me to look at the CI errors? I'm not very familiar with these issues. Thank you.

I think that should fix the tests.

libcxx/include/__bit/bit_ceil.h
32 ↗(On Diff #540939)
43 ↗(On Diff #540939)

You forgot _LIBCPP_NODISCARD_EXT here.

Joy12138 updated this revision to Diff 542832.Jul 21 2023, 2:42 AM
Joy12138 marked 2 inline comments as done.

Fix CI

libcxx/include/__bit/bit_ceil.h
32 ↗(On Diff #540939)

Seems we can't use brace-initialization of scalar types in C++03.

I tried to fix using:

return static_cast<_Tp>(1) << __n;

@philnik It seems the CI errors related to constexpr for bit_ceil? I'm not sure how to fix. Do you have any solutions?

@philnik It seems the CI errors related to constexpr for bit_ceil? I'm not sure how to fix. Do you have any solutions?

I think the problem is that you changed the if constexpr to an if. The simplest solution is probably to change it back to an if constexpr and constraining __bit_ceil to _LIBCPP_STD_VER >= 17.

Joy12138 updated this revision to Diff 543256.Jul 23 2023, 12:10 AM

Rebase and fix bit_ceil

This revision was landed with ongoing or failed builds.Sep 11 2023, 8:42 PM
This revision was automatically updated to reflect the committed changes.