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

Tue Jun 3 14:21:02 UTC 2025

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

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

> 312:         // mask value for which iff all corresponding input bits are set then bit compression
> 313:         // will result in a -ve value, therefore this case negates the possibility of
> 314:         // strictly non-negative bit compression result.

Grammar was a little off. I think we can say it in fewer words.
Suggestion:

        // Case B.1 The mask value range includes -1, hence we may use all bits,
        //          the result has the whole value range.

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

> 326:         // optimistic upper bound of result i.e. all the bits other than leading zero bits
> 327:         // can be assumed holding 1 value.
> 328:         assert(mask_type->lo_as_long() >= 0, "");

Suggestion:

        assert(mask_type->lo_as_long() >= 0, "");
        // Case B.3 Mask value range only includes non-negative values. Since all integral
        // types honours an invariant that TypeInteger._lo <= TypeInteger._hi, thus computing
        // leading zero bits of upper bound of mask value will allow us to ascertain
        // optimistic upper bound of result i.e. all the bits other than leading zero bits
        // can be assumed holding 1 value.

Have the assert first, like a condition. Then the comments follow from it and make sense immediately.

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

> 337:       // compression result will never be a -ve value and we can safely set the
> 338:       // lower bound of bit compression to zero.
> 339:       lo = result_bit_width == mask_bit_width ? lo : 0L;

Fixed grammar a little.
Suggestion:

      // If the number of bits required to for the mask value range is less than the
      // full bit width of the integral type, then the MSB bit is guaranteed to be zero,
      // thus the compression result will never be a -ve value and we can safely set the
      // lower bound of the bit compression to zero.
      lo = result_bit_width == mask_bit_width ? lo : 0L;

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

> 367:       hi = src_type->hi_as_long() >= 0 ? src_type->hi_as_long() : hi;
> 368:       // Tightening upper bound of bit compression as per Rule 3.
> 369:       hi = result_bit_width < mask_bit_width ? MIN2((jlong)((1UL << result_bit_width) - 1L), hi) : hi;

// As per Rule 1, bit compression packs the source bits corresponding to
      // set mask bits

This says something, but does not really explain the rest of the sentence:

      // set mask bits, hence for a non-negative input, result of compression will
      // always be less that equal to input.

Plus: input could be both `mask` or `src`. I would be specific, and just talk about `src`.

Also: I don't really see how the conclusion in this sentence follows from its assumption.
How exactly does some bits not participating really ensure that the value is not greater?

        // set. If a mask bit corresponding to set input bit is zero then that input bit will
        // not take part in bit compression, which means that maximum possible result value
        // can never be greater than non-negative input.

I think I know what you are trying to say, it just sounds a little vague.

It also smells like this `Lemma 1` might be easier proved by a proof of contradiction.
I need to take a break now, but I'll see if I can come up with something a bit clearer later.

Here my suggestion:
Suggestion:

      if (src_type->hi_as_long() >= 0) {
        // Lemma 1: For strictly non-negative src, the result of the compression will never be
        // greater than src.
        // Proof: Since src is a non-negative value, its most significant bit is always 0.
        // Thus even if the corresponding MSB of the mask is one, the result will be a +ve
        // value. <the actual proof>
        hi = src_type->hi_as_long();
      }

      if (result_bit_width < mask_bit_width) {
        // Rule 3:
        // We can further constrain the upper bound of bit compression if the number of bits
        // which can be set to 1 is less than the maximum number of bits of integral type.
        hi = MIN2((jlong)((1UL << result_bit_width) - 1L), hi);
      }

test/hotspot/jtreg/compiler/c2/gvn/TestBitCompressValueTransform.java line 307:

> 305:         }
> 306:         Asserts.assertEQ(0, res);
> 307:     }

Like I mentioned in the email. I contributed this test, it would be nice if you could give me credits by making me a contributor to this issue.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2123857262
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2123864703
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2123881395
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2123983125
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2123992004