RFR: 8349563: Improve AbsNode::Value() for integer types [v3]

[Jasmine Karthikeyan]

On Wed, 28 May 2025 11:57:51 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Jasmine Karthikeyan has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:
>> 
>>  - Replace uabs usage with ABS
>>  - Merge branch 'master' into abs-value
>>  - Merge
>>  - Improve AbsNode::Value
>
> @jaskarth Nice work! I have a few comments below.
> 
> One is about more randomized tests. I'm thinking about something like this:
> 
> - compute `res = Math.abs(x)`
> - Truncate `x` with randomly produced bounds from Generators, like this: `x = Math.max(lo, Math.min(hi, x))`.
> - Below, add all sorts of comparisons with random constants, like this: `if (res < CON) { sum += 1; }`. If the output range is wrong, this could wrongly constant fold, and allow us to catch that.
> - Then fuzz the generated method a few times with random inputs for `x`, and check that the sum and res value are the same for compiled and interpreted code.
> 
> I hope that makes sense :)
> This is currently my best method to check if ranges are correct, and I think it is quite important because often tests are only written with constants in mind, but less so with ranges, and then we mess up the ranges because it is just too tricky.
> 
> This is an example, where I asked someone to try this out as well:
> https://github.com/openjdk/jdk/pull/23089/files#diff-12bebea175a260a6ab62c22a3681ccae0c3d9027900d2fdbd8c5e856ae7d1123R404-R422

@eme64 Thanks for the review and comments! The method of checking for constant folding with if statements and range filtering you mentioned is pretty clever. I've adapted it to the test and added it to the PR. Let me know what you think!

> src/hotspot/share/opto/subnode.cpp line 1947:
> 
>> 1945: 
>> 1946:     return IntegerType::make(ABS(t->get_con()));
>> 1947:   }
> 
> We used `uabs` before, what prevents you from doing that now? I guess you would need a templated version, hmm. Could be worth looking into creating one.

There was an earlier discussion in the review: https://github.com/openjdk/jdk/pull/23685#discussion_r1972735806
Essentially, the implementation of `uabs` relies on converting ints/longs from signed to unsigned which is implementation defined until C++20. I believe the implementation works as expected on most platforms, but to be cautious I thought it would be better to just handle it manually to avoid any potential problems. We should revisit when we're at C++20 ;)

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

PR Comment: https://git.openjdk.org/jdk/pull/23685#issuecomment-3030523657
PR Review Comment: https://git.openjdk.org/jdk/pull/23685#discussion_r2181570566