-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8350896: Integer/Long.compress gets wrong type from CompressBitsNode::Value #23947
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
/label add hotspot-compiler-dev |
|
👋 Welcome back jbhateja! A progress list of the required criteria for merging this PR into |
|
@jatin-bhateja This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 1421 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
@jatin-bhateja |
Webrevs
|
eme64
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
test/hotspot/jtreg/compiler/c2/TestBitCompressValueTransform.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/c2/TestBitCompressValueTransform.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/c2/TestBitCompressValueTransform.java
Outdated
Show resolved
Hide resolved
|
Hi @eme64 , I have addressed your comments, let me know if you need further clarifications. |
eme64
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jatin-bhateja Thanks for the updates! I have a few more requests :)
test/hotspot/jtreg/compiler/c2/TestBitCompressValueTransform.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/compiler/c2/TestBitCompressValueTransform.java
Outdated
Show resolved
Hide resolved
|
Hi @eme64 , I have addressed and responded to your comments, please verify. |
test/hotspot/jtreg/compiler/c2/TestBitCompressValueTransform.java
Outdated
Show resolved
Hide resolved
eme64
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
test/hotspot/jtreg/compiler/c2/TestBitCompressValueTransform.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jatin-bhateja Thanks for the updates!
I think I now understand everything except this, so we are making good progress 😊
// For upper bound estimation of result value range with a constant input we
// pessimistically pick max_int value to prevent incorrect constant folding
// in case input equals above estimated lower bound.
hi = src_type->hi_as_long() == lo ? hi : src_type->hi_as_long();
Can you please explain it with an example, and walk me through the steps to the incorrect constant folding?
Let's assume the following
Earlier _hi value of the result value range was set to _hi value of the source value range.
If the result bit width was less than the maximum bit width of the integral type, in that case both _hi and _lo values were being set to MIN_VALUE resulting into a constant value. |
|
@jatin-bhateja Thanks for explaining the case with I would still like to see a argument/proof of line Let's assume Here the counter-example: Running it with: You can see the the exception is wrongly thrown, and you can see the wrong type of the If I run it in intepreter only, I do not see the exception: We got this code wrong before, and now again. How can we gain confidence that it will be correct on the next attempt? My Opinion? |
|
@jatin-bhateja Well, |
For me to approve this code, you will have to do more than that. I will need:
|
|
@jatin-bhateja This operation is non-trivial, I expect the level of coverage to be on par with #23089. If you want to have a quick fix, I suggest removing all the logic and simply returning the bottom type. |
@eme64 , glad you are picking that up. I don't want to comment on KnownBits on this PR I will add my review suggestions on #17508, Need a lil time to refresh memory, but we are excited to contribute to it. I think for this bug fix, its better to be safe for now, let me update the revision and test. |
|
Hi @eme64 , can you kindly run this latest version through your testing, please. |
|
@jatin-bhateja I think I'd rather wait until you have more thorough testing and the proofs I asked for, otherwise I would need to run the testing twice ;) |
I have added comments in the code which give sufficient details, let me know if you still need more explanation |
|
Quick note on C2 Integral types:-
Verification ensures that the lattice is symmetrical around the centerline, i.e., a semi-lattice.
e.g., if TypeInt t1 = {lo:10, hi:100} and TypeInt t2 = {lo:1, hi:20}, then
|
|
Hi @TobiHartmann . Please let me know if it's good to land in. |
|
Thanks, testing looks good now! I'm out for the rest of the week and can review only next week. |
TobiHartmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me and I think you addressed all the comments that Emanuel had. Let's wait for another day or two in case someone else wants to take a look as well.
In the meantime, please request approval for integration into JDK 25 since we are know at RDP 2:
https://openjdk.org/jeps/3#Fix-Request-Process
|
I am not sure about JDK 25 approval for these changes. Can you do simple fix for JDK 25 as @merykitty suggested: "I suggest removing all the logic and simply returning the bottom type" ? Will it be the same complexity? Will it affect performance (and how much)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kvn, Apart from couple of lines of fix, patch mainly re-structured existing handling and added additional comments and proofs along with an exhaustive test.
Will it be the same complexity? Will it affect performance (and how much)?
Effective code changes are very minimal, in general value-constracting optimizations can prune the control paths or constant fold dependent expressions, thereby reducing the JIT code size, which has other side effects on inlining decisions. For infrequently taken paths, C2 anyway injects Uncommon traps.
Should I check this into jdk-mainline and then prepare minimal fix without comments / re-structuring (if needed) for jdk25u?
Hi @TobiHartmann, Can you please re-verify the latest version of patch and approve if all tests are green. |
Sure, I'll re-run testing. How did you find the issue that you fixed in the latest commit? |
|
@vnkozlov Given that this is an old issue already affecting JDK 19, maybe we should just defer to JDK 26 for now and then backport to JDK 25u only once the fix is stable? |
Yes, I agree. Please replace fix request with defer request in JBS bug report. |
Hi @TobiHartmann , On a re-review I found this incompatibility between code and comments, its not a correctness issue but trying to constrain the result value range further :-) Hi @vnkozlov , On performance front, I see 500x improvement with and without patch for following micro.
Path length i.e. dynamic instruction count is significantly more with baseline. |
TobiHartmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looks good to me. I'll re-run testing with the latest changes. @jatin-bhateja could you please adjust your "Fix request" comment in JBS to be a "Defer request", see https://openjdk.org/jeps/3#Bug-Deferral-Process
|
Testing is all clean. I think this is good to go into JDK 26. |
|
/integrate |
Thanks @TobiHartmann , integrating it. |
|
Going to push as commit b02c125.
Your commit was automatically rebased without conflicts. |
|
@jatin-bhateja Pushed as commit b02c125. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
Thank you, @jatin-bhateja , for providing performance numbers. |
eme64
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jatin-bhateja @vnkozlov @TobiHartmann
The TemplateFramework fuzzer found a bug in this bugfix:
#28062
It is quite subtle, and would have been hard to spot in the review. But we could have done a better job with tests.
| // result.lo = 0 | ||
| if (maskcon != -1L) { | ||
| int bitcount = population_count(static_cast<julong>(bt == T_INT ? maskcon & 0xFFFFFFFFL : maskcon)); | ||
| hi = (1UL << bitcount) - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace 1UL -> 1ULL for Windows.
| // Rule 3: | ||
| // We can further constrain the upper bound of bit compression if the number of bits | ||
| // which can be set(one) is less than the maximum number of bits of integral type. | ||
| hi = MIN2((jlong)((1UL << result_bit_width) - 1L), hi); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace 1UL -> 1ULL for Windows.
| src = Math.max(BOUND_LO_L, Math.min(src, BOUND_HI_L)); | ||
| long res = Long.compress(src, mask); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and in all other similar tests:
It seems we are only creating "random input ranges" for src, and not for mask. Bummer


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
Progress
Issue
Reviewers
Contributors
<[email protected]>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23947/head:pull/23947$ git checkout pull/23947Update a local copy of the PR:
$ git checkout pull/23947$ git pull https://git.openjdk.org/jdk.git pull/23947/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23947View PR using the GUI difftool:
$ git pr show -t 23947Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23947.diff
Using Webrev
Link to Webrev Comment