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

Wed Mar 12 08:45:57 UTC 2025

On Fri, 7 Mar 2025 17:37:36 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 Thanks for looking into this! I left a first set of comments :)

Primarily, it is about these issues:
- We need good comments, preferably even proofs. Because we got things wrong the last time, and there were no comments/proofs. It's difficult to get this sort of arithmetic transformation right, and it is hard to review. Proofs help to think through all the steps carefully.
- Test coverage: I would like to see some more randomized cases of input ranges.

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;

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

> 264:     if ( opc == Op_CompressBits) {
> 265:       int mask_max_bw;
> 266:       int max_bw = bt == T_INT ? 32 : 64;

Should there be an assert somewhere that `bt` is either `T_INT` or `T_LONG`?

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

> 268:       // strictly non-negative result value range.
> 269:       if ((mask_type->lo_as_long() < 0L && mask_type->hi_as_long() >= -1L)) {
> 270:         mask_max_bw = max_bw;

This sounds like it should be the `else` case, where we can prove nothing special. I would put it last.

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

> 273:       // a +ve value range.
> 274:       } else if (mask_type->hi_as_long() < -1L) {
> 275:         mask_max_bw = max_bw - 1;

I would say something more explicit, like this:

Case 2) The mask range does not include -1, which is the only case where all bits are set in the mask.
Hence, at least one bit is not set in the mask, and so after compression the most significant bit, i.e.
the sign bit is zero, and the compression result must thus be non-negative.

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

> 276:       } else {
> 277:       // Case 3) Mask value range only includes +ve values, this can again be
> 278:       // used to ascertain known Zero bits of resultant value.

I would put this case as the first, swapping it with Case 1).
And I would say something more explicit like this:
`Case 3) The mask value range is non-negative. Hence, the mask has at least one zero bit.`

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

> 278:       // used to ascertain known Zero bits of resultant value.
> 279:         assert(mask_type->lo_as_long() >= 0, "");
> 280:         jlong clz = count_leading_zeros(mask_type->hi_as_long());

Suggestion:

        jlong clz = count_leading_zeros(mask_type->hi_as_long());
        // The mask has at least clz leading zeros, and hence also the compression
        // result must have at least clz leading zeros.

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 ;)

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

> 283:       }
> 284: 
> 285:       lo = mask_max_bw == max_bw ? lo : 0L;

Suggestion:

      // If we have found that we have at least one zero bit in the mask, and hence at least
      // one leading zero in the compression result, we know that the compression result
      // is non-negative, and we can update lo accordingly.
      assert(lo <= 0L, "we are only narrowing the type");
      lo = mask_max_bw == max_bw ? lo : 0L;

Not sure if the assert is ok, but I think so.

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

> 287:       // result value range is primarily dependent on true count
> 288:       // of participating mask value. Thus bit compression can never
> 289:       // result into a value greater than original value.

To me this is not clear "inherently" 🙈 
Maybe there could be a quick proof like this:

1) src < 0, result < 0: only if mask == -1, and so src == result.
2) src < 0, result >= 0: src < result.
3) src >= 0, ...

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

> 292:       // input equals lower bound of mask value range.
> 293:       hi = src_type->hi_as_long() == lo ? hi : src_type->hi_as_long();
> 294:       hi = mask_max_bw < max_bw ? (1L << mask_max_bw) - 1 : hi;

I need some more explanation / correctness proof here. I find it difficult to immediately see all possible cases 🙈

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

> 24: /*
> 25:  * @test
> 26:  * @key stress randomness

Suggestion:

I don't see any randomness or stressing in this test, correct?

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

> 25:  * @test
> 26:  * @key stress randomness
> 27:  * @requires vm.compiler2.enabled & os.simpleArch == "x64"

Can you please remove this restriction, so we can also run this test with other compilers and platforms?
You can always restrict the IR rules instead ;)

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

> 97:     @IR (counts = { IRNode.COMPRESS_BITS, " 0 "} , failOn = { IRNode.UNSTABLE_IF_TRAP }, applyIfCPUFeature = { "bmi2", "true" })
> 98:     public long test4(long value) {
> 99:         long filter_bits = value & 0xF;

It would be nice if we had some test cases with random ranges. So at least have something that puts in a random mask value. But alternatively also something that could span arbitrary ranges, maybe using `min/max` for clamping.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23947#pullrequestreview-2677433845
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r1990898233
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r1990890991
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r1990909653
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r1990916737
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r1990923240
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r1990924402
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r1990895378
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r1990937109
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r1990947893
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r1990950676
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r1990954143
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r1990881804
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r1990958427