BUG: left bit shift undefined behavior#29539
Conversation
|
I re-triggered the failing CI job, which was unrelated to this PR. |
|
Ping @seiko2plus - this PR touches code you originally wrote back in 2021. |
seberg
left a comment
There was a problem hiding this comment.
I am fine with just putting it in, I do think the << is clearer to read, but this is so targeted, that I guess it just doesn't matter.
| const __m256i sigb = npyv_setall_s64(1LL << 63); | ||
| q = _mm256_srl_epi64(_mm256_add_epi64(q, sigb), shf1); | ||
| q = _mm256_sub_epi64(q, _mm256_srl_epi64(sigb, shf1)); | ||
| const __m256i sbit = npyv_setall_s64(0x8000000000000000); |
There was a problem hiding this comment.
I suppose it is relatively clear from the sbit context, but I wonder if we can at least keep a
| const __m256i sbit = npyv_setall_s64(0x8000000000000000); | |
| const __m256i sbit = npyv_setall_s64(0x8000000000000000 /* 1 << 63 */); |
(since I don't have a better idea, I assume 1ULL << 63 doesn't warn, but then we would also need to cast back to long long probably, so dunno that is better)
There was a problem hiding this comment.
I did try it out with 1ULL << 63 and it removed the warning, but was trying to be consistent with other bit shift operations.
There was a problem hiding this comment.
Sebastian's suggestion just adds a comment, I think it's fine to add. Probably also worth adding in sse/operators.h as well:
sse/operators.h
36: const __m128i sbit = npyv_setall_s64(0x8000000000000000);
220: const __m128i sbit = npyv_setall_s64(0x8000000000000000);
There's also this in sse/arithmetic.h:
sse/arithmetic.h
254: const __m128i sigb = npyv_setall_s64(1LL << 63);
Although maybe this one is fine because it's a signed long long?
There was a problem hiding this comment.
Nope, that one also gets flagged by UBSAN. I also have a fix in for that in this PR.
There was a problem hiding this comment.
Ah of course, got a little tunnel-vision there.
Let's merge this as-is, we can clean up the constants to be formatted nicely later. Fixing the UB is way more important.
|
Thanks @m-clare! |
sse/arithmetic.handavx2/arithmetic.hsigbtosbit) to match those insse/operations.havx2/operations.hfor the same operation with a vector argument (see snippet below).compiler_sanitizers.ymlworkflow should succeed without errors.From
sse/operations.hL34:The pattern throughout these implementations is to use the hex representation of the value, which I've done here. Not sure if it would be a better option to use the
limitlibrary instead.