RFR: 8344026: [s390x] ubsan failure: signed integer overflow in c1_LIRGenerator_s390.cpp

[Martin Doerr]

On Fri, 15 Nov 2024 11:10:42 GMT, Amit Kumar <amitkumar at openjdk.org> wrote:

>> src/hotspot/share/c1/c1_LIRGenerator.cpp line 533:
>> 
>>> 531:         if (right->is_constant()) {
>>> 532:           jint c = right->as_jint();
>>> 533:           if (c > 0 && c < max_jint) {
>> 
>> This prevents platform specific optimizations for negative `c`. E.g. multiplication by -1 could be strength reduced to a negation.
>
> I didn't get it. How this will affect -1 case ? 
> 
> I see that this is implementation for `strength_reduce_multiply`: 
> 
> bool LIRGenerator::strength_reduce_multiply(LIR_Opr left, jint c, LIR_Opr result, LIR_Opr tmp) {
>   assert(left != result, "should be different registers");
>   if (is_power_of_2(c + 1)) {
>     __ shift_left(left, log2i_exact(c + 1), result);
>     __ sub(result, left, result);
>     return true;
>   } else if (is_power_of_2(c - 1)) {
>     __ shift_left(left, log2i_exact(c - 1), result);
>     __ add(result, left, result);
>     return true;
>   }
>   return false;
> }
> 
> which will return false in case of `-1`.  
> 
> Basically even without my change, `c = -1` will set `did_strength_reduce` to `false`.

I'm not talking about existing code. I'm talking about possibilities which you prevent. With your code `strength_reduce_multiply` will no longer be called with negative `c` preventing possible optimizations inside of it.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22144#discussion_r1843611142