RFR: 8350896: Integer/Long.compress gets wrong type from CompressBitsNode::Value [v9]

[Jatin Bhateja]

On Tue, 3 Jun 2025 09:14:36 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix aarch64 failure
>
> src/hotspot/share/opto/intrinsicnode.cpp line 267:
> 
>> 265:     //  mask = 0xEFFFFFFF (constant mask)
>> 266:     //  result.hi = 0x7FFFFFFF
>> 267:     //  result.lo = 0
> 
> Should shit not go inside the `CompressBits` scope?
> `Hi` -> `lo`
> 
> `Result.Hi = popcount(1 << mask_bits - 1)`
> Does not look right. Is this not the wrong way around?
> Just repeating code here also does not make sense. Either give a reason in english, or just drop the duplication if it is indeed trivail.
> 
> I would also do the case distinction a bit clearer:
> 
> If mask == -1 -> all ones -> just returns src:
>   result.lo = type_min  (happens if src = type_min)
> 
> Question: does that not mean we could just return the input type of `src`?
> 
> If mask != -1 -> at least one zero in mask -> result cannot be negative:
>   result.lo = 0
> 
> 
> But if we are doing this with the comments, then why not just create an `if-else` block, and add the comments inside each block?

> ```
> If mask == -1 -> all ones -> just returns src:
> 
> Question: does that not mean we could just return the input type of `src`?

This is incorrect, bit compression simply compacts the bits corresponding to set mask bits, thus for all true mask, if the mask of the source is 1, along with some other set bits, but if it includes atleast one unset (zero) bit then result will be a +ve value and not same as src.

> src/hotspot/share/opto/intrinsicnode.cpp line 292:
> 
>> 290:     //     To compute minimum result value we assume all but last read source bit as zero,
>> 291:     //     this is because sign bit of result will always be set to 1 while other bit
>> 292:     //     corresponding to set mask bit should be zero.
> 
> I don't understand, are you talking about `lo` if `mask < 0`? Don't we just keep `lo = type_min`, which is always ok?

Correct, that is what the comment explains, all bits apart from MSB bit are zero type_min 0x80000000 (int) and similarly for long...

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2204754423
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2204754550