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

[Emanuel Peter]

On Wed, 12 Mar 2025 08:01:15 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Hi All,
>> 
>> This bugfix patch fixes incorrect value computation for Integer/Long. compress APIs.
>> 
>> Problems occur with a constant input and variable mask where the input's value is equal to the lower bound of the mask value., In this case, an erroneous value range estimation results in a constant value. Existing value routine first attempts to constant fold the compression operation if both input and compression mask are constant values; otherwise, it attempts to constrain the value range of result based on the upper and lower bounds of mask type.
>> 
>> New IR test covers the issue reported in the bug report along with a case for value range based logic pruning.
>> 
>> Kindly review and share your feedback.
>> 
>> Best Regards,
>> Jatin
>
> src/hotspot/share/opto/intrinsicnode.cpp line 265:
> 
>> 263:   if (!mask_type->is_con()) {
>> 264:     if ( opc == Op_CompressBits) {
>> 265:       int mask_max_bw;
> 
> Suggestion:
> 
>       // Pattern: Integer/Long.compress(src_type, mask_type)
>       int mask_max_bw;

Can you also say what the meaning of `mask_max_bw` is? Possibly a more expressive name would help here too.

> src/hotspot/share/opto/intrinsicnode.cpp line 283:
> 
>> 281:         clz = bt == T_INT ? clz - 32 : clz;
>> 282:         mask_max_bw = max_bw - clz;
>> 283:       }
> 
> Can you please put the comments for cases 1-3 either consistently before the condition, or after the condition with inlining? I would vote for inside each condition with indentation, so just like case 3), except 2 spaces indented ;)

Why not start with the "nice" case 3) first, where we know that the range is positive, and so even after compression we cannot get negative values?

What does this mean `only includes +ve values`?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r1990900245
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r1990907216