Skip to content

fix(neon): unsigned bitwise shifts are never called #1266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
serge-sans-paille merged 4 commits into from
Mar 6, 2026
Merged

fix(neon): unsigned bitwise shifts are never called #1266

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ea1608e
Unsigned bitwise shifts are never called on neon
AntoinePrv Mar 4, 2026
fdb201c
Add assert for shift positive
AntoinePrv Mar 4, 2026
33f01b6
Fix neon rhsift uint64_t
AntoinePrv Mar 4, 2026
4314f07
Use all_of in shifts_all_positive
AntoinePrv Mar 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 45 additions & 16 deletions include/xsimd/arch/xsimd_neon.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

#include <algorithm>
#include <array>
#include <cassert>
#include <complex>
#include <tuple>
#include <type_traits>
Expand Down Expand Up @@ -2371,6 +2372,17 @@ namespace xsimd
return bitwise_lshift(lhs, n, ::xsimd::detail::int_sequence<Is...>());
}
}

template <class A, class T>
XSIMD_INLINE bool all_positive(batch<T, A> const& b) noexcept
Copy link
Copy Markdown
Contributor

@serge-sans-paille serge-sans-paille Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just xsimd::all(b >= 0)

Copy link
Copy Markdown
Contributor Author

@AntoinePrv AntoinePrv Mar 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does not work because kernel::all on which it depends is declared below.
Would also require compare utilities to be declared.

This is not the only case where I find it hard to reuse other functions, with possibly circular include loops.
With all the SFINAE, forward declaring all of them is not very maintainable.
(In C++17 we could get rid of all SFINAE in favor of if constexpr and have simple signatures that we can easily forward declare).

Copy link
Copy Markdown
Contributor

@serge-sans-paille serge-sans-paille Mar 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the C++17 simplification. But that's for another day. meanwhile you could at least store to a buffer and call std::all_of ?

Copy link
Copy Markdown
Contributor Author

@AntoinePrv AntoinePrv Mar 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@serge-sans-paille I'm happy to do the changes, but not sure it will be for the best.
What is the objective? Less assertion code? No extra visible symbol (all_positive)?

If we want to get rid of all_positive then we would need at least this in all functions. Possibly more with #ifdef NDEBUG to avoid warnings given [[maybe_unused]] is C++17.

std::array<T, batch<T, A>> tmp = {};
rhs.store(tmp.data());
assert(std::all_of(tmp.begin(), tmp.end(), [](auto x) { return x >= 0; }));

Or do you mean to use std::all_of inside all_positive?

{
for (std::size_t k = 0; k < b.size; ++k)
{
if (b.get(k) < 0)
return false;
}
return true;
}
}

template <class A, class T>
Expand All @@ -2382,9 +2394,11 @@ namespace xsimd
}

template <class A, class T, detail::enable_sized_unsigned_t<T, 1> = 0>
XSIMD_INLINE batch<T, A> bitwise_lshift(batch<T, A> const& lhs, batch<as_signed_integer_t<T>, A> const& rhs, requires_arch<neon>) noexcept
XSIMD_INLINE batch<T, A> bitwise_lshift(batch<T, A> const& lhs, batch<T, A> const& rhs, requires_arch<neon>) noexcept
{
return vshlq_u8(lhs, rhs);
// Blindly converting to signed since out of bounds shifts are UB anyways
Comment thread
AntoinePrv marked this conversation as resolved.
assert(detail::all_positive(rhs));
return vshlq_u8(lhs, vreinterpretq_s8_u8(rhs));
}

template <class A, class T, detail::enable_sized_signed_t<T, 1> = 0>
Expand All @@ -2394,9 +2408,11 @@ namespace xsimd
}

template <class A, class T, detail::enable_sized_unsigned_t<T, 2> = 0>
XSIMD_INLINE batch<T, A> bitwise_lshift(batch<T, A> const& lhs, batch<as_signed_integer_t<T>, A> const& rhs, requires_arch<neon>) noexcept
XSIMD_INLINE batch<T, A> bitwise_lshift(batch<T, A> const& lhs, batch<T, A> const& rhs, requires_arch<neon>) noexcept
{
return vshlq_u16(lhs, rhs);
// Blindly converting to signed since out of bounds shifts are UB anyways
Comment thread
AntoinePrv marked this conversation as resolved.
assert(detail::all_positive(rhs));
return vshlq_u16(lhs, vreinterpretq_s16_u16(rhs));
}

template <class A, class T, detail::enable_sized_signed_t<T, 2> = 0>
Expand All @@ -2406,9 +2422,11 @@ namespace xsimd
}

template <class A, class T, detail::enable_sized_unsigned_t<T, 4> = 0>
XSIMD_INLINE batch<T, A> bitwise_lshift(batch<T, A> const& lhs, batch<as_signed_integer_t<T>, A> const& rhs, requires_arch<neon>) noexcept
XSIMD_INLINE batch<T, A> bitwise_lshift(batch<T, A> const& lhs, batch<T, A> const& rhs, requires_arch<neon>) noexcept
{
return vshlq_u32(lhs, rhs);
// Blindly converting to signed since out of bounds shifts are UB anyways
assert(detail::all_positive(rhs));
return vshlq_u32(lhs, vreinterpretq_s32_u32(rhs));
}

template <class A, class T, detail::enable_sized_signed_t<T, 4> = 0>
Expand All @@ -2418,9 +2436,11 @@ namespace xsimd
}

template <class A, class T, detail::enable_sized_unsigned_t<T, 8> = 0>
XSIMD_INLINE batch<T, A> bitwise_lshift(batch<T, A> const& lhs, batch<as_signed_integer_t<T>, A> const& rhs, requires_arch<neon>) noexcept
XSIMD_INLINE batch<T, A> bitwise_lshift(batch<T, A> const& lhs, batch<T, A> const& rhs, requires_arch<neon>) noexcept
{
return vshlq_u64(lhs, rhs);
// Blindly converting to signed since out of bounds shifts are UB
assert(detail::all_positive(rhs));
return vshlq_u64(lhs, vreinterpretq_s64_u64(rhs));
}

template <class A, class T, detail::enable_sized_signed_t<T, 8> = 0>
Expand Down Expand Up @@ -2618,9 +2638,11 @@ namespace xsimd
}

template <class A, class T, detail::enable_sized_unsigned_t<T, 1> = 0>
XSIMD_INLINE batch<T, A> bitwise_rshift(batch<T, A> const& lhs, batch<as_signed_integer_t<T>, A> const& rhs, requires_arch<neon>) noexcept
XSIMD_INLINE batch<T, A> bitwise_rshift(batch<T, A> const& lhs, batch<T, A> const& rhs, requires_arch<neon>) noexcept
{
return vshlq_u8(lhs, vnegq_s8(rhs));
// Blindly converting to signed since out of bounds shifts are UB anyways
assert(detail::all_positive(rhs));
return vshlq_u8(lhs, vnegq_s8(vreinterpretq_s8_u8(rhs)));
}

template <class A, class T, detail::enable_sized_signed_t<T, 1> = 0>
Expand All @@ -2630,9 +2652,11 @@ namespace xsimd
}

template <class A, class T, detail::enable_sized_unsigned_t<T, 2> = 0>
XSIMD_INLINE batch<T, A> bitwise_rshift(batch<T, A> const& lhs, batch<as_signed_integer_t<T>, A> const& rhs, requires_arch<neon>) noexcept
XSIMD_INLINE batch<T, A> bitwise_rshift(batch<T, A> const& lhs, batch<T, A> const& rhs, requires_arch<neon>) noexcept
{
return vshlq_u16(lhs, vnegq_s16(rhs));
// Blindly converting to signed since out of bounds shifts are UB anyways
assert(detail::all_positive(rhs));
return vshlq_u16(lhs, vnegq_s16(vreinterpretq_s16_u16(rhs)));
}

template <class A, class T, detail::enable_sized_signed_t<T, 2> = 0>
Expand All @@ -2642,9 +2666,11 @@ namespace xsimd
}

template <class A, class T, detail::enable_sized_unsigned_t<T, 4> = 0>
XSIMD_INLINE batch<T, A> bitwise_rshift(batch<T, A> const& lhs, batch<as_signed_integer_t<T>, A> const& rhs, requires_arch<neon>) noexcept
XSIMD_INLINE batch<T, A> bitwise_rshift(batch<T, A> const& lhs, batch<T, A> const& rhs, requires_arch<neon>) noexcept
{
return vshlq_u32(lhs, vnegq_s32(rhs));
// Blindly converting to signed since out of bounds shifts are UB anyways
assert(detail::all_positive(rhs));
return vshlq_u32(lhs, vnegq_s32(vreinterpretq_s32_u32(rhs)));
}

template <class A, class T, detail::enable_sized_signed_t<T, 4> = 0>
Expand All @@ -2654,9 +2680,12 @@ namespace xsimd
}

template <class A, class T, detail::enable_sized_unsigned_t<T, 8> = 0>
XSIMD_INLINE batch<T, A> bitwise_rshift(batch<T, A> const& lhs, batch<as_signed_integer_t<T>, A> const& rhs, requires_arch<neon>) noexcept
XSIMD_INLINE batch<T, A> bitwise_rshift(batch<T, A> const& lhs, batch<T, A> const& rhs, requires_arch<neon> req) noexcept
{
return vshlq_u64(lhs, neg(rhs, neon {}).data);
// Blindly converting to signed since out of bounds shifts are UB anyways
assert(detail::all_positive(rhs));
using S = std::make_signed_t<T>;
return vshlq_u64(lhs, neg(batch<S, A>(vreinterpretq_s64_u64(rhs)), req).data);
}

template <class A, class T, detail::enable_sized_signed_t<T, 8> = 0>
Expand Down
7 changes: 5 additions & 2 deletions include/xsimd/arch/xsimd_neon64.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#ifndef XSIMD_NEON64_HPP
#define XSIMD_NEON64_HPP

#include <cassert>
#include <complex>
#include <cstddef>
#include <tuple>
Expand Down Expand Up @@ -1209,9 +1210,11 @@ namespace xsimd
}

template <class A, class T, detail::enable_sized_unsigned_t<T, 8> = 0>
XSIMD_INLINE batch<T, A> bitwise_rshift(batch<T, A> const& lhs, batch<as_signed_integer_t<T>, A> const& rhs, requires_arch<neon64>) noexcept
XSIMD_INLINE batch<T, A> bitwise_rshift(batch<T, A> const& lhs, batch<T, A> const& rhs, requires_arch<neon64>) noexcept
{
return vshlq_u64(lhs, vnegq_s64(rhs));
// Blindly converting to signed since out of bounds shifts are UB anyways
assert(detail::all_positive(rhs));
return vshlq_u64(lhs, vnegq_s64(vreinterpretq_s64_u64(rhs)));
}

template <class A, class T, detail::enable_sized_signed_t<T, 8> = 0>
Expand Down
Loading