Skip to content
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.

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

Fix invalid shift warning in vsli_n implementation. #1253

Merged
merged 1 commit into from
Jan 2, 2025

Conversation

Syonyk
Copy link
Contributor

@Syonyk Syonyk commented Jan 2, 2025

Fixes: #1231

Per the ARMv8 manual, the valid range of shifts for the vector SLI operations is "0 to the element width in bits minus 1." The existing SIMDe implementation creates an invalid shift in the case of 0, as the shifts are (element width - n) - so, for a 0-bit shift on a 64-bit value, the shift is 64. This is undefined per the C spec, and leads to compiler warnings on build. Examples of the error:

[1/2] Compiling C object test/arm/neon/sli_n-native-c.p/sli_n.c.o
In file included from ../test/arm/neon/sli_n.c:4:
../test/arm/neon/sli_n.c: In function ‘test_simde_vsli_n_s32’:
../simde/arm/neon/sli_n.h:132:62: warning: right shift count >= width of type [-Wshift-count-overflow]
  132 |                       simde_vdup_n_u32((UINT32_C(0xffffffff) >> (32 - n)))), \
      |                                                              ^~
../simde/arm/neon/sli_n.h:118:32: note: in expansion of macro ‘simde_vsli_n_u32’
  118 |     simde_vreinterpret_s32_u32(simde_vsli_n_u32( \
      |                                ^~~~~~~~~~~~~~~~
../test/arm/neon/sli_n.c:311:26: note: in expansion of macro ‘simde_vsli_n_s32’
  311 |     simde_int32x2_t r0 = simde_vsli_n_s32(a, b, 0);
      |                          ^~~~~~~~~~~~~~~~
../test/arm/neon/sli_n.c: In function ‘test_simde_vsli_n_s64’:
../simde/arm/neon/sli_n.h:158:63: warning: right shift count >= width of type [-Wshift-count-overflow]
  158 |                                 (UINT64_C(0xffffffffffffffff) >> (64 - n)))), \
      |                                                               ^~
../simde/arm/neon/sli_n.h:144:32: note: in expansion of macro ‘simde_vsli_n_u64’
  144 |     simde_vreinterpret_s64_u64(simde_vsli_n_u64( \
      |                                ^~~~~~~~~~~~~~~~
../test/arm/neon/sli_n.c:435:26: note: in expansion of macro ‘simde_vsli_n_s64’
  435 |     simde_int64x1_t r0 = simde_vsli_n_s64(a, b, 0);
      |    

This fix changes the sli_n shift operations to work properly for the valid range of values, shifting ((element width - 1) - n), with a modified constant value to generate the same results (7f... instead of ff...).

UINT64_C(0xffffffffffffffff) >> (64 - n)
UINT64_C(0x7fffffffffffffff) >> (63 - n)

For all valid n values of (0 to width - 1), this now generates a valid, C-spec-compliant shift value, and eliminates the warning about undefined behavior.

While the existing tests all pass with the change, a number of the tests have been modified (and have new constant values generated) to properly exercise and demonstrate the "n == 0" shift case correctness. These test vectors were generated on an ARMv9 system (Google Compute Engine C4A system), and pass on x86 hardware as well.

Copy link
Collaborator

@mr-c mr-c left a comment

Choose a reason for hiding this comment

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

Thank you very much @Syonyk !

@mr-c mr-c enabled auto-merge (rebase) January 2, 2025 19:51
Per the ARMv8 manual, the valid range of shifts for the vector SLI
operations is "0 to the element width in bits minus 1."  The existing
SIMDe implementation creates an invalid shift in the case of 0, as
the shifts are (element width - n) - so, for a 0-bit shift on a
64-bit value, the shift is 64.  This is undefined per the C spec, and
leads to compiler warnings on build.

This fix changes the sli_n shift operations to work properly for the
valid range of values, shifting ((element width - 1) - n), with a
modified constant value to generate the same results (7f... instead
of ff...).

While the existing tests all pass with the change, a number of the
tests have been modified (and have new constant values generated) to
properly exercise and demonstrate the "n == 0" shift case.  These
test vectors were generated on an ARMv9 system (Google Compute Engine
C4A system), and pass on x86 hardware as well.
auto-merge was automatically disabled January 2, 2025 20:14

Head branch was pushed to by a user without write access

@mr-c mr-c enabled auto-merge (rebase) January 2, 2025 20:28
@mr-c mr-c disabled auto-merge January 2, 2025 23:37
@mr-c mr-c merged commit 8067442 into simd-everywhere:master Jan 2, 2025
95 of 96 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SLI "shift by zero" creates an invalid shift on x86
2 participants