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

[Jatin Bhateja]

On Wed, 2 Apr 2025 09:44:06 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Review comments resolutions
>
> 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.

> 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.

> src/hotspot/share/opto/intrinsicnode.cpp line 298:
> 
>> 296:       // in case input equals above estimated lower bound.
>> 297:       hi = src_type->hi_as_long() == lo ? hi : src_type->hi_as_long();
>> 298:       hi = max_mask_bit_width < mask_bit_width ? (1L << max_mask_bit_width) - 1 : hi;
> 
> I still don't understand your comment here. For example, I don't see a `max_int` in the code... And I also don't see anything that deals with constants in the code explicitly.
> 
> And similarly as above, how do we ensure that `hi` is not raised accidentally?

'hi' is initialized to 'max_int' and nothing before line 297 is modifying 'hi'.
'lo' is either 'min_int' or '0',  for constant input set to 'min_int' we want prevent incorrect constant folding of result as for any integral constant lo and hi bounds points to same value. 

For zero input we have added special value transform for compress/expand which is related to the fix.

> 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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2036631948
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2036631830
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2036631742
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2036632791