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

[Emanuel Peter]

On Mon, 12 May 2025 02:35:50 GMT, Jasmine Karthikeyan <jkarthikeyan at openjdk.org> wrote:

>> Hi all,
>> This is a small patch that improves the implementation of Value() for `AbsINode` and `AbsLNode` by returning the absolute value of the input range. Most of the logic is trivial except for the special case where `_lo == jint_min/jlong_min` which must return the entire type range when encountered, for which I've added a small proof in the comments. I've also added some unit tests and updated the file to limit IR check platforms with more granularity.
>> 
>> Thoughts and reviews would be appreciated!
>
> 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

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.

src/hotspot/share/opto/subnode.cpp line 1956:

> 1954:     // - As abs(type_min+1) == type_max and for all n from type_min+1 to hi, abs(n) <= type_max, the upper bound must be type_max.
> 1955: 
> 1956:     return IntegerType::TYPE_DOMAIN;

Nice, I like proofs! I was wondering if we can make it a little conciser, but up to you which verison you want.
Suggestion:

    // Both type_min and type_max are in the output type, hence we return the whole type domain:
    // - type_min is in t                                -> abs(type_min)   = type_min
    // - type_min+1 is in t, because t is not a constant -> abs(type_min+1) = type_max
    return IntegerType::TYPE_DOMAIN;

Meh, but yours is a bit more complete. Maybe better keep yours.

src/hotspot/share/opto/subnode.cpp line 1960:

> 1958: 
> 1959:   NativeType lo_abs = ABS(t->_lo);
> 1960:   NativeType hi_abs = ABS(t->_hi);

Suggestion:

  // Knowing that min_type is not in t, we know there is no overflow.
  NativeType lo_abs = ABS(t->_lo);
  NativeType hi_abs = ABS(t->_hi);

test/hotspot/jtreg/compiler/c2/irTests/TestIRAbs.java line 333:

> 331:         // [-9, -2] => [2, 9]
> 332:         return Math.abs(-((in & 7) + 2)) > 9;
> 333:     }

Could we have some randomized cases here too? Or do we already have them somewhere?

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

PR Review: https://git.openjdk.org/jdk/pull/23685#pullrequestreview-2874674288
PR Review Comment: https://git.openjdk.org/jdk/pull/23685#discussion_r2111638516
PR Review Comment: https://git.openjdk.org/jdk/pull/23685#discussion_r2111655487
PR Review Comment: https://git.openjdk.org/jdk/pull/23685#discussion_r2111658190
PR Review Comment: https://git.openjdk.org/jdk/pull/23685#discussion_r2111666504