[libc++] Explicitly convert to masks in SIMD code

[philnik777]

This makes it clearer when we use masks and avoids MSan complaining.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

This makes it clearer when we use masks and avoids MSan complaining.

---

Full diff: https://github.com/llvm/llvm-project/pull/107983.diff

2 Files Affected:

• (modified) libcxx/include/__algorithm/mismatch.h (+4-4)
• (modified) libcxx/include/__algorithm/simd_utils.h (+50-27)

diff --git a/libcxx/include/__algorithm/mismatch.h b/libcxx/include/__algorithm/mismatch.h
index 0fae7f6e3fe323..043901d32798aa 100644
--- a/libcxx/include/__algorithm/mismatch.h
+++ b/libcxx/include/__algorithm/mismatch.h
@@ -77,7 +77,7 @@ __mismatch_vectorized(_Iter __first1, _Iter __last1, _Iter __first2) {
       }
 
       for (size_t __i = 0; __i != __unroll_count; ++__i) {
-        if (auto __cmp_res = __lhs[__i] == __rhs[__i]; !std::__all_of(__cmp_res)) {
+        if (auto __cmp_res = std::__as_mask(__lhs[__i] == __rhs[__i]); !std::__all_of(__cmp_res)) {
           auto __offset = __i * __vec_size + std::__find_first_not_set(__cmp_res);
           return {__first1 + __offset, __first2 + __offset};
         }
@@ -89,7 +89,7 @@ __mismatch_vectorized(_Iter __first1, _Iter __last1, _Iter __first2) {
 
     // check the remaining 0-3 vectors
     while (static_cast<size_t>(__last1 - __first1) >= __vec_size) {
-      if (auto __cmp_res = std::__load_vector<__vec>(__first1) == std::__load_vector<__vec>(__first2);
+      if (auto __cmp_res = std::__as_mask(std::__load_vector<__vec>(__first1) == std::__load_vector<__vec>(__first2));
           !std::__all_of(__cmp_res)) {
         auto __offset = std::__find_first_not_set(__cmp_res);
         return {__first1 + __offset, __first2 + __offset};
@@ -106,8 +106,8 @@ __mismatch_vectorized(_Iter __first1, _Iter __last1, _Iter __first2) {
     if (static_cast<size_t>(__first1 - __orig_first1) >= __vec_size) {
       __first1 = __last1 - __vec_size;
       __first2 = __last2 - __vec_size;
-      auto __offset =
-          std::__find_first_not_set(std::__load_vector<__vec>(__first1) == std::__load_vector<__vec>(__first2));
+      auto __offset = std::__find_first_not_set(
+          std::__as_mask(std::__load_vector<__vec>(__first1) == std::__load_vector<__vec>(__first2)));
       return {__first1 + __offset, __first2 + __offset};
     } // else loop over the elements individually
   }
diff --git a/libcxx/include/__algorithm/simd_utils.h b/libcxx/include/__algorithm/simd_utils.h
index 56518dafa3193b..ec9840f60d87c0 100644
--- a/libcxx/include/__algorithm/simd_utils.h
+++ b/libcxx/include/__algorithm/simd_utils.h
@@ -116,42 +116,65 @@ template <class _VecT, class _Iter>
   }(make_index_sequence<__simd_vector_size_v<_VecT>>{});
 }
 
-template <class _Tp, size_t _Np>
-[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI bool __all_of(__simd_vector<_Tp, _Np> __vec) noexcept {
-  return __builtin_reduce_and(__builtin_convertvector(__vec, __simd_vector<bool, _Np>));
+template <size_t _Np>
+[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI bool __all_of(__simd_vector<bool, _Np> __vec) noexcept {
+  return __builtin_reduce_and(__vec);
 }
 
 template <class _Tp, size_t _Np>
-[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI size_t __find_first_set(__simd_vector<_Tp, _Np> __vec) noexcept {
-  using __mask_vec = __simd_vector<bool, _Np>;
+[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI auto __as_mask(__simd_vector<_Tp, _Np> __vec) noexcept {
+  static_assert(!is_same<_Tp, bool>::value, "vector type should not be a bool!");
+  return __builtin_convertvector(__vec, __simd_vector<bool, _Np>);
+}
 
-  // This has MSan disabled du to https://github.com/llvm/llvm-project/issues/85876
-  auto __impl = [&]<class _MaskT>(_MaskT) _LIBCPP_NO_SANITIZE("memory") noexcept {
-#  if defined(_LIBCPP_BIG_ENDIAN)
-    return std::min<size_t>(
-        _Np, std::__countl_zero(__builtin_bit_cast(_MaskT, __builtin_convertvector(__vec, __mask_vec))));
-#  else
-    return std::min<size_t>(
-        _Np, std::__countr_zero(__builtin_bit_cast(_MaskT, __builtin_convertvector(__vec, __mask_vec))));
-#  endif
-  };
-
-  if constexpr (sizeof(__mask_vec) == sizeof(uint8_t)) {
-    return __impl(uint8_t{});
-  } else if constexpr (sizeof(__mask_vec) == sizeof(uint16_t)) {
-    return __impl(uint16_t{});
-  } else if constexpr (sizeof(__mask_vec) == sizeof(uint32_t)) {
-    return __impl(uint32_t{});
-  } else if constexpr (sizeof(__mask_vec) == sizeof(uint64_t)) {
-    return __impl(uint64_t{});
+// This uses __builtin_convertvector around the __builtin_shufflevector to work around #107981.
+template <size_t _Np>
+[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI __simd_vector<bool, 8>
+__extend_vector(__simd_vector<bool, _Np> __vec) noexcept {
+  using _VecT = __simd_vector<bool, _Np>;
+  if constexpr (_Np == 4) {
+    return __builtin_convertvector(
+        __builtin_shufflevector(__vec, _VecT{}, 0, 1, 2, 3, 4, 5, 6, 7), __simd_vector<bool, 8>);
+  } else if constexpr (_Np == 2) {
+    return std::__extend_vector(
+        __builtin_convertvector(__builtin_shufflevector(__vec, _VecT{}, 0, 1, 2, 3), __simd_vector<bool, 4>));
+  } else if constexpr (_Np == 1) {
+    return std::__extend_vector(
+        __builtin_convertvector(__builtin_shufflevector(__vec, _VecT{}, 0, 1), __simd_vector<bool, 2>));
   } else {
-    static_assert(sizeof(__mask_vec) == 0, "unexpected required size for mask integer type");
+    static_assert(sizeof(_VecT) == 0, "Unexpected vector size");
+  }
+}
+
+template <size_t _Np>
+[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI auto __to_int_mask(__simd_vector<bool, _Np> __vec) {
+  if constexpr (_Np < 8) {
+    return std::__bit_cast<uint8_t>(std::__extend_vector(__vec));
+  } else if constexpr (_Np == 8) {
+    return std::__bit_cast<uint8_t>(__vec);
+  } else if constexpr (_Np == 16) {
+    return std::__bit_cast<uint16_t>(__vec);
+  } else if constexpr (_Np == 32) {
+    return std::__bit_cast<uint32_t>(__vec);
+  } else if constexpr (_Np == 64) {
+    return std::__bit_cast<uint64_t>(__vec);
+  } else {
+    static_assert(sizeof(__simd_vector<bool, _Np>) == 0, "Unexpected vector size");
     return 0;
   }
 }
 
-template <class _Tp, size_t _Np>
-[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI size_t __find_first_not_set(__simd_vector<_Tp, _Np> __vec) noexcept {
+template <size_t _Np>
+[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI size_t __find_first_set(__simd_vector<bool, _Np> __vec) noexcept {
+#  if defined(_LIBCPP_BIG_ENDIAN)
+  return std::min<size_t>(_Np, std::__countl_zero(std::__to_int_mask(__vec)));
+#  else
+  return std::min<size_t>(_Np, std::__countr_zero(std::__to_int_mask(__vec)));
+#  endif
+}
+
+template <size_t _Np>
+[[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI size_t __find_first_not_set(__simd_vector<bool, _Np> __vec) noexcept {
   return std::__find_first_set(~__vec);
 }
 

[vitalybuka]

Probably unrelated, this is the only algorithm patch in the range https://lab.llvm.org/buildbot/#/builders/85/builds/1657/steps/11/logs/stdio

[philnik777]

@thurstond Please comment on a PR when you revert it. I didn't see the revert until now. Also, what's the path forward here? The pre-commit CI passed, so whatever you're seeing is most likely not a supported configuration. It seems like I can't see the logs anymore, so I don't even know what the failure is.

CC @vitalybuka, since you commented before.

[vitalybuka]

@thurstond Please comment on a PR when you revert it. I didn't see the revert until now.

Github UI makes it confusing, some may have an impression that

on PR will notify the user.

It would be nice to have a policy about that. Sometimes I create a new issue on revert, other times I set personal notifications to check In a day or so if the author acknowledged the revert.

Also, what's the path forward here? The pre-commit CI passed, so whatever you're seeing is most likely not a supported configuration.

Pre-commit CI covers only a fraction of officially libc++ supported configurations, e.g. the arm is not covered at all. Also why passing libc++ under sanitizer is not a supported configuration?

It seems like I can't see the logs anymore, so I don't even know what the failure is.

We know the bot name "sanitizer-aarch64-linux-bootstrap-hwasan", I can re-run
#121352 on the bot without commit.

https://lab.llvm.org/buildbot/#/builders/55/builds/4900

Crashing source:
ranges.tar.gz

[thurstond]

@thurstond Please comment on a PR when you revert it. I didn't see the revert until now.

@philnik777 Sorry, I didn't realize github doesn't automatically notify about reverts. I will comment on PRs in the future.

---

We know the bot name "sanitizer-aarch64-linux-bootstrap-hwasan", I can re-run
#121352 on the bot without commit.

Thanks @vitalybuka!

[vitalybuka]

So this is a crash in compiler triggered by PR.

Also, what's the path forward here?

If we think this is test only specific issue (I doubt it is), we can disable them with HWASAN.
I believe it will likely hit down stream users, so we better to fix compiler before reland.

[vitalybuka]

Created #121365 with reproducer. Hopefully we resolve this quickly.

BTW. Seems there is existing guideline https://llvm.org/docs/DeveloperPolicy.html#id19 : "It is customary to respond to the original commit email mentioning the revert." But I would prefer some stronger wording.

[philnik777]

Pre-commit CI covers only a fraction of officially libc++ supported configurations, e.g. the arm is not covered at all. Also why passing libc++ under sanitizer is not a supported configuration?

It looks like you're not aware of the libc++ pre-commit CI, which is different from the rest of the project. We do have arm coverage. In fact, we have over 60 different configurations across various platforms that we support. We don't have any hwasan coverage in there though, which means that we're not supporting it officially. That's simply the policy. If you want to change that, feel free to add a new configuration to the test matrix for it. I'm happy to help with that if there are problems.

@thurstond Please comment on a PR when you revert it. I didn't see the revert until now.

@philnik777 Sorry, I didn't realize github doesn't automatically notify about reverts. I will comment on PRs in the future.

No worries!

[vitalybuka]

It looks like you're not aware of the libc++ pre-commit CI, which is different from the rest of the project. We do have arm coverage. In fact, we have over 60 different configurations across various platforms that we support. We don't have any hwasan coverage in there though, which means that we're not supporting it officially. That's simply the policy. If you want to change that, feel free to add a new configuration to the test matrix for it. I'm happy to help with that if there are problems.

Sorry, probably I am not aware.
I assumed pre-commit CI is https://github.com/llvm/llvm-project/actions/runs/12563251328/job/35025625406

And this is worklow file https://github.com/llvm/llvm-project/blob/1b09bb7b0fa1c101021dfd00ffc47f4689f085df/.github/workflows/libcxx-build-and-test.yaml

How to tell if it's running aarch64?

[philnik777]

Sorry, probably I am not aware. I assumed pre-commit CI is https://github.com/llvm/llvm-project/actions/runs/12563251328/job/35025625406

And this is worklow file https://github.com/llvm/llvm-project/blob/1b09bb7b0fa1c101021dfd00ffc47f4689f085df/.github/workflows/libcxx-build-and-test.yaml

How to tell if it's running aarch64?

This is most of the pre-commit CI, but there is also https://buildkite.com/llvm-project/libcxx-ci/builds/37688, which mostly runs the different platforms. IDK how to see that after the fact though, or whether there is even any UI hint that it ever ran.