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

[Emanuel Peter]

On Thu, 10 Apr 2025 06:52:04 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> src/hotspot/share/opto/intrinsicnode.cpp line 266:
>> 
>>> 264:     if ( opc == Op_CompressBits) {
>>> 265:       // Pattern: Integer/Long.compress(src_type, mask_type)
>>> 266:       int max_mask_bit_width;
>> 
>> Suggestion:
>> 
>>       int result_bit_width;
>> 
>> Is this bit width not about the result? It is really not about the mask.
>> 
>> Example:
>> `mask_type->hi_as_long() < -1L`
>> 
>> Here, the mask has the uppermost bit set, and so the bit width of it is the maximum 32 / 64 bits.
>> 
>> But we still can deduce that the result has one leading zero bit, and so the bit width of the result is either 31 or 63.
>
> Your interpretation is correct. But this is indeed the max_mask_bit_width, which constrains the upper bound of the result value.

What I am saying is that calling it `max_mask_bit_width` seems **incorrect**. Your `Case 2` checks if the sign bit is set. In that case, `max_mask_bit_width` has to be `32` or `64`. But then you set it `max_mask_bit_width = mask_bit_width - 1;`. So now it would be `31` or `63`. That would be incorrect for the mask. But it would be correct for the result.

>> src/hotspot/share/opto/intrinsicnode.cpp line 292:
>> 
>>> 290:       // compression result will never be a -ve value and we can safely set the
>>> 291:       // lower bound of the result value range to zero.
>>> 292:       lo = max_mask_bit_width == mask_bit_width ? lo : 0L;
>> 
>> Can you please add an assert that we are not making `lo` worse than what we already have? Someone may insert optimizations above that set `lo > 0`, and then you may lower it again here.
>> Suggestion:
>> 
>>       assert(lo < 0, "we should not lower the value of lo");
>>       lo = max_mask_bit_width == mask_bit_width ? lo : 0L;
>
> We are already initializing 'lo' to min_jint/jlong value upfront and nothing before this line is modifying its value. Since initialization and use both are part of this function hense adding an assertion looks redundant. We generally add assertions to set constraints under which a logic is implemented.

Your response is saying that the code is currently correct. That is true as far as I can see now.
But my worry is about future changes. And an assert can also help the reader be sure that things happen correctly without having to verify manually, which is a lot of effort. So for me it would not be redundant.

>> src/hotspot/share/opto/intrinsicnode.cpp line 391:
>> 
>>> 389:      return TypeInteger::zero(bt);
>>> 390:   }
>>> 391: 
>> 
>> Is this change related to the PR title? And do you have any tests for it?
>
> Zero value transform is related to fix added new test points.

Nit: It looks really like an extension rather than bug fix... or did we fold these before as well?
Would you backport these changes?

Since you added tests, I'm willing to let it go in.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2052242676
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2052251238
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2052246850