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

[Emanuel Peter]

On Mon, 21 Apr 2025 06:40:42 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8350896
>  - Review comments resolutions
>  - Review comments resolutions
>  - 8350896: Integer/Long.compress gets wrong type from CompressBitsNode::Value

Here are my responses :)

I'm still worried about updating `hi` and `lo` without some kind of assert or proof that it does not widen accidentally. I think we can prove with some effort that they do not widen currently. But then someone else comes along and modifies the code and then they might not realize the subtle proof we have in our heads currently.

And I doubt that we have good tests that would catch such regressions, if the range was suddenly a little wider than before.

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

> 281:         assert(mask_type->lo_as_long() >= 0, "");
> 282:         // Here, result of clz is w.r.t to long argument, hence for integer argument
> 283:         // we explicitly subtract 32 from the result.

The comments are not at the correct line. It does not seem to refer to the line above or below. I would say either move it up to the `Case 3` comments, or move it down to the specific line that actually subtracts `32`.

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

> 297:       // in case input equals above estimated lower bound.
> 298:       hi = src_type->hi_as_long() == lo ? hi : src_type->hi_as_long();
> 299:       hi = max_mask_bit_width < mask_bit_width ? (1L << max_mask_bit_width) - 1 : hi;

Somehow the conversation disappeared with changes, so I'll screenshot it it here for context:

![image](https://github.com/user-attachments/assets/e4e5d05b-3fdd-4e67-ab0a-8164783648bc)

> 'hi' is initialized to 'max_int' and nothing before line 297 is modifying 'hi'.

That is not super apparent to me, at least not at first. I'd have to know the code really well. A quick scan gives me lines like `hi = (1UL << bitcount) - 1;` above. Sure, that is on an unrelated path, but I'd have to check like 20+ lines above. I don't have that attention span ;)

So maybe some better naming here could help.

Can you convert your explanations into comments in the code? Or even better asserts, so we are even more sure that it is correct now, and also once others make changes here that might invalidate your assumptions around `hi` and `lo`?

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

> 1: /*
> 2:  * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
> 3:  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.

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

> There are operation specific tests under compiler/c2 let keep it this way.

But this is about a Value optimization, which generally belong under the gvn directory. Just because we used to put all tests in one directory does not mean we should continue that practice ;)

We should probably at some point clean up all the tests, and move them to better directories. Putting them already to better directories will reduce the backport issues later on, so please put it somewhere more specific.

@jatin-bhateja I'm not very happy when people just "resolve" a conversation unilaterally. Can you please avoid that in the future?

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23947#pullrequestreview-2781004906
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2052244785
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2052259016
PR Review Comment: https://git.openjdk.org/jdk/pull/23947#discussion_r2052248407