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

[Emanuel Peter]

On Wed, 2 Apr 2025 07:39:02 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:
> 
>   Review comments resolutions

@jatin-bhateja Thanks for the updates! I have a few more requests :)

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.

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

> 272:       } else if (mask_type->hi_as_long() < -1L) {
> 273:         // Case 2) Mask value range is less than -1, this indicates presence of at least
> 274:         // one zero bit in the mask value, there by constraining the result of compression

Suggestion:

        // one zero bit in the mask value, thereby constraining the result of compression

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;

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?

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?

test/hotspot/jtreg/compiler/c2/TestBitCompressValueTransform.java line 2:

> 1: /*
> 2:  * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.

Ah, I just noticed the test directory. I think we can put it in a more specific location.

test/hotspot/jtreg/compiler/c2/TestBitCompressValueTransform.java line 29:

> 27:  * @library /test/lib /
> 28:  * @summary C2: wrong result: Integer/Long.compress gets wrong type from CompressBitsNode::Value.
> 29:  * @run main/othervm -Xbatch -XX:-TieredCompilation -XX:CompileThresholdScaling=0.3 compiler.c2.TestBitCompressValueTransform

Do you really need the flags here? The IR framework already makes sure that compilation happens, and then we execute the test again. So `Xbatch` may not be necessary to reproduce the bug. And same for `TieredCompilation`. Maybe we actually don't need any flags, but please check!

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23947#pullrequestreview-2734951364
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2024480144
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2024462231
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2024485135
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2024491349
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2024260171
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2024257750
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2024256227