BUG: Fix INT_MIN % -1 to return 0 for all signed integer types #29893
Conversation
- Explicitly check for INT_MIN % -1 in scalar tail loops for fmod and remainder kernels. - Set result to 0 to avoid undefined behavior and match NumPy/Python expectations. - Ensures correct, portable behavior on all platforms (e.g., PPC64LE).
| for (; len > 0; --len, ++src1, ++dst1) { | ||
| const npyv_lanetype_@sfx@ a = *src1; | ||
| *dst1 = a % scalar; | ||
| /* Fix PPC64LE: INT_MIN % -1 should be 0, not INT_MIN */ |
There was a problem hiding this comment.
Thanks the fix looks good! I don't think the comment is necessary since this is indeed undefined behavior, it is surprising that UBSAN didn't point it out ever?
I think this should also add a npy_set_floatstatus_overflow().
You mentioned that the test_overflow() test is fixed by this and was skipped? I guess I am missing where it is actually skipped (should that skip be removed), or was it just "skipped dealing with it" and it failed locally?
I am very surprised that the test doesn't notice the lack of overflow warning, since it seems to test length 1 arrays and check for overflow?
Would be nice to understand that if you have time, or I'll have to have a second look.
There was a problem hiding this comment.
Thanks, @seberg!
I’ll remove the comment.
Regarding the test_overflow() behavior — on our Power systems, these tests behave differently depending on the hardware:
On Power9, they are skipped automatically since VSX4 is not available.
On Power10, the same tests fail due to overflow warnings raised by the hardware.
When npy_set_floatstatus_overflow() is included, the additional warning handling causes these tests (test_overflows, test_signed_division_overflow) in TestDivisionIntegerOverflowsAndDivideByZero to fail on PPC64LE.
To keep the CI clean and consistent, I’ve added a conditional skip for these specific tests using:
@pytest.mark.skipif(IS_PPC64LE, reason="Overflow warnings differ on Power (ppc64le)")
so they are skipped only on PPC64LE platforms.
If you think including npy_set_floatstatus_overflow() is necessary, I can keep the skip logic separate from this PR (BUG: Fix INT_MIN % -1 to return 0 for all signed integer types) and open a small follow-up PR focused on the test adjustments for PPC64LE.
There was a problem hiding this comment.
Sorry, I got the wrong code path: the warning is necessary only for divmod and there it already is of course.
So I feel we can remove the comment, but let me just make a suggestion and apply that and we are done. Thanks!
There was a problem hiding this comment.
I got the wrong code path
I think we should revisit this source. The latest compilers should auto-vectorize remainder/divmod when VSX4 is enabled, so we may be able to remove these kernels.
|
Thank you, @sandeepgupta12, for working on this bug, and @seberg for the review. |
…#29893) * BUG: Fix INT_MIN % -1 to return 0 for all signed integer types - Explicitly check for INT_MIN % -1 in scalar tail loops for fmod and remainder kernels. - Set result to 0 to avoid undefined behavior and match NumPy/Python expectations. - Ensures correct, portable behavior on all platforms (e.g., PPC64LE). * Apply suggestion from @seberg --------- Co-authored-by: Sebastian Berg <[email protected]>
…#29893) * BUG: Fix INT_MIN % -1 to return 0 for all signed integer types - Explicitly check for INT_MIN % -1 in scalar tail loops for fmod and remainder kernels. - Set result to 0 to avoid undefined behavior and match NumPy/Python expectations. - Ensures correct, portable behavior on all platforms (e.g., PPC64LE). * Apply suggestion from @seberg --------- Co-authored-by: Sebastian Berg <[email protected]>
BUG: Fix INT_MIN % -1 to return 0 for all signed integer types (#29893)
…#29893) * BUG: Fix INT_MIN % -1 to return 0 for all signed integer types - Explicitly check for INT_MIN % -1 in scalar tail loops for fmod and remainder kernels. - Set result to 0 to avoid undefined behavior and match NumPy/Python expectations. - Ensures correct, portable behavior on all platforms (e.g., PPC64LE). * Apply suggestion from @seberg --------- Co-authored-by: Sebastian Berg <[email protected]>
…#29893) * BUG: Fix INT_MIN % -1 to return 0 for all signed integer types - Explicitly check for INT_MIN % -1 in scalar tail loops for fmod and remainder kernels. - Set result to 0 to avoid undefined behavior and match NumPy/Python expectations. - Ensures correct, portable behavior on all platforms (e.g., PPC64LE). * Apply suggestion from @seberg --------- Co-authored-by: Sebastian Berg <[email protected]>
4 participants
Summary of changes
Explicitly handle the case INT_MIN % -1 in scalar tail loops for fmod and remainder kernels.
Set the result to 0 to avoid undefined behavior and align with NumPy/Python expected semantics.
Ensures portable, correct behavior across all architectures, including POWER10 (ppc64le).
Background / Motivation
On POWER10 systems, several SIMD tests were failing due to integer overflow when evaluating INT_MIN % -1, an undefined operation in C.
Python defines this operation to return 0, and this change updates NumPy’s low-level implementation to match that behavior.
Issue Reference
Fixes: #29731 — [POWER10] SIMD tests failing with integer overflow (previously skipped)
cc: @seiko2plus @rgommers @ghatwala