RFR: 8347645: C2: XOR bounded value handling blocks constant folding [v27]

[Emanuel Peter]

On Wed, 12 Feb 2025 19:48:29 GMT, Johannes Graham <duke at openjdk.org> wrote:

>> C2 does not eliminate XOR nodes with constant arguments. This has a noticeable effect on `Long.expand` with a constant mask, on architectures that don't have instructions equivalent  to `PDEP` to be used in an intrinsic.
>> 
>> This patch demonstrates a potential fix to the problem, but there might well be better ways to do it.
>
> Johannes Graham has updated the pull request incrementally with one additional commit since the last revision:
> 
>   formatting, remove commented tests

@j3graham Thanks for taking this on! It's great to see someone clean this up and make sure all the cases optimize as expected!

src/hotspot/share/opto/addnode.cpp line 986:

> 984: 
> 985: template<class S, class U>
> 986: static S calc_xor_max(const S hi_0, const S hi_1) {

Can we please have a more expressive name? Having `max` in the name can be a little confusing, as it is its own operation, which is not relevant here it seems. What we are really finding is the `hi` of the type after the `xor`. So why not name it `calculate_hi_after_xor`?

src/hotspot/share/opto/addnode.cpp line 995:

> 993: 
> 994:   // We want to find a value that has all 1 bits everywhere up to and including
> 995:   // the highest bits set in r0->_hi as well as r1->_hi. For this,we can take the next

Suggestion:

  // the highest bits set in r0->_hi as well as r1->_hi. For this, we can take the next

src/hotspot/share/opto/addnode.cpp line 1010:

> 1008:   if( r0 == TypeInt::BOOL && ( r1 == TypeInt::ONE
> 1009:                                || r1 == TypeInt::BOOL))
> 1010:     return TypeInt::BOOL;

It looks to me like this case should be covered by `calc_xor_max` below. Do we have any IR tests that verify that this still gets optimized as before?

src/hotspot/share/opto/addnode.cpp line 1046:

> 1044: jint XorINode::calc_max(const jint hi_0, const jint hi_1)  {
> 1045:   return calc_xor_max<jint, juint>(hi_0, hi_1);
> 1046: }

What is this method used for? Only by the tests? Why not use `calc_xor_max` in the tests directly?

src/hotspot/share/opto/addnode.cpp line 1063:

> 1061:   // Result of xor can only have bits sets where any of the
> 1062:   // inputs have bits set. lo can always become 0.
> 1063: 

Hmm, I'm not super happy with this comment. It feels like somehow the naming of `calc_xor_max` and the checks around its use are not very "clear" or apparent on a first read. The comments help a little...

The comment about `lo can always become 0.` is not generally true, I mean if it is already below 0 which we have not YET checked at the time of the comment, then we cannot just change it to zero.

Maybe we want to refactor this logic here.
Really you are checking if we have an unsigned case here. And then you are checking if you can constrain the `hi` to the possibly active bits.

I mean currently your `calc_xor_max` would even assert if the inputs are negative, so I feel the method name should reflect that expectation as well.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23089#pullrequestreview-2614273871
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1954079856
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1954080333
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1954085505
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1954073522
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1954097305