BUG: Don't produce undefined behavior for a << b if b >= bitsof(a) #13739
Conversation
|
CI failure is due to this assert: https://github.com/gcc-mirror/gcc/blob/9d0507742960aa9f2b99bc6e9584655ecc611792/gcc/expmed.c#L2349 Which can be reprod on godbolt here. An extremely minimal repro is here. @juliantaylor, could you look at reporting this upstream? |
|
@seberg: We have no precedent in that file for functions templated over all integer types. The best we have are What names would you propose? |
|
Oh,had not checked, but doesn't sound too bad, the normal type code should suffice, e.g. |
|
Realized that there's precedent for Unfortunately, I strongly suspect this is going to fail CI again (and if it doesn't, then I think we have a performance problem). I think the only fix here is to add a |
|
Looks like this hit a problem on macOS as well. Somehow the FPU flag gets set, but only on computations with |
| #ifdef @TYPE@_left_shift_needs_clear_floatstatus | ||
| // For some reason, our macOS CI sets an "invalid" flag here, but only | ||
| // for some types. | ||
| npy_clear_floatstatus_barrier((char*)dimensions); |
There was a problem hiding this comment.
I have no idea why this happens. Is our macOS run x86, or something else? Do we have a resident macOS expert?
There was a problem hiding this comment.
I have no idea, was thinking about putting it in godbold, but it is quite a lot of code around it (and I am not sure it is likely to show anything in any case).
@charris, I think I am good with this as is. But you had a look at it before and know npy_math stuff better. The floatstatus thing (and I guess the compile time check) are a bit unfortunate though...
e8fbd2f to
6d4eb24
Compare
|
Coverage stats are just bad because we can't measure coverage for setup.py. ARM CI now passes, because It means 64-bit |
| NPY_INPLACE npy_@u@@type@ | ||
| npy_rshift@u@@c@(npy_@u@@type@ a, npy_@u@@type@ b) | ||
| { | ||
| if (NPY_LIKELY((size_t)b < sizeof(a) * CHAR_BIT)) { |
There was a problem hiding this comment.
Do we want to deal with the possibility that b is negative?
There was a problem hiding this comment.
Not that currently the shift value is cast to unsigned and masked, at least on my hardware.
In [1]: a = array([1,2,3,4])
In [2]: a << -1
Out[2]:
array([-9223372036854775808, 0, -9223372036854775808,
0])
There was a problem hiding this comment.
I think there was a comment in the old/new code about that as well. Maybe we can push it off to another PR? That seems the easiest way forward? We can ignore the codecov failures.
|
Fixes #2249 |
|
With the caveat that negative shifts still need to be handled, I think this is good to go as is |
|
I'm OK with this going in. Would be good to have a release note about changed behavior in case it affects somebody downstream. |
|
Waiting on resolution of #13908 to know how to add a release note |
|
@seberg Ping. |
|
@charris thanks. Let me merge the towncrier related PRs (since nobody complained), and then I will add a short release note to this. Other than that I remember this as being good (although that compiler bug is a bit annoying). |
55cf805 to
9f258d8
Compare
|
OK, I rebased this (it was a bit messy) and added a release note (we will have to modify the towncrier template a bit before the end, but it works). Master seems to have added |
|
restarting tests |
This: * Inlines the macros in loops.c.src * Replaces 8 with `CHAR_BIT `. The `NPY_SIZEOF_*` macros are not used here because it's too much work to apply them to the signed types, and they expand to the same thing anyway. * Removes the reduce loop specializations which likely no one cares about * Uses pytest.mark.parametrize to shorten the test
This avoids duplication, as suggested in review. The suffices `l`, `ull` etc are for consistency with `strtoull` and friends. `h` and `uhh` are inspired from the C99 printf specifiers.
…rror Needed to make CI pass
9f258d8 to
6cf6ece
Compare
|
Thanks @eric-wieser |
This is a continuation of gh-7473. Fixes gh-2449