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

[Emanuel Peter]

On Mon, 2 Jun 2025 17:58:09 GMT, Jatin Bhateja <jbhateja 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
>
> Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix aarch64 failure

Thanks for all the comment updates! I had a few minute to look into it, and will add more later!

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?

src/hotspot/share/opto/intrinsicnode.cpp line 272:

> 270:       int bitcount = population_count(static_cast<julong>(bt == T_INT ? maskcon & 0xFFFFFFFFL : maskcon));
> 271:       hi = maskcon == -1L ? hi : (1UL << bitcount) - 1;
> 272:       lo = maskcon == -1L ? lo : 0L;

It could be nice to have a proper `if-else` here, and add the comments to each scope, rather than above. That would allow you to avoid duplicating the code above in the comments.

src/hotspot/share/opto/intrinsicnode.cpp line 274:

> 272:       lo = maskcon == -1L ? lo : 0L;
> 273:     } else {
> 274:     // Case A.2 bit expansion:-

I would put the assert for `Op_ExpandBits` above, so that it is immediately clear that this matches.

src/hotspot/share/opto/intrinsicnode.cpp line 278:

> 276:     //     Result.Hi = mask, optimistically assuming all source bits
> 277:     //     read starting from least significant bit positions are 1.
> 278:     //     Result.Lo = 0

Suggestion:

    //     Result.Lo = 0, because at least one bit in mask is zero.

src/hotspot/share/opto/intrinsicnode.cpp line 288:

> 286:     //     For constant mask strictly less than zero, maximum result value will be
> 287:     //     same as mask value with its sign bit flipped, assuming all but last read
> 288:     //     source bits are set to 1.

Suggestion:

    //     For constant mask strictly less than zero, the maximum result value will be
    //     the same as the mask value with its sign bit flipped, assuming all source bits but the last
    //     are set to 1.

src/hotspot/share/opto/intrinsicnode.cpp line 298:

> 296:     //    result.hi = 0xEFFFFFFF ^ 0x80000000 = 0x6FFFFFFF
> 297:     //    result.lo = 0x80000000
> 298:     //

Same here: why not do a proper `if-else`, and add the comments to each scope directly?

src/hotspot/share/opto/intrinsicnode.cpp line 300:

> 298:     //
> 299:       assert(opc == Op_ExpandBits, "");
> 300:       hi = maskcon >= 0L ? maskcon : maskcon ^ lo;

If you are already touching this line: `maskcon ^ lo` is really a bit hairy. It should really be `maskcon ^ type_min(bt)`, and then you add a comment right there that it is a sign flip.

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

PR Review: https://git.openjdk.org/jdk/pull/23947#pullrequestreview-2891441258
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2123225738
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2123228042
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2123229716
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2123235763
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2123240806
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2123231362
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2123248474