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

[Emanuel Peter]

On Thu, 20 Feb 2025 16:25:48 GMT, Johannes Graham <duke at openjdk.org> wrote:

>> An interaction between xor bounds optimization and constant folding resulted in xor over constants not being optimized. 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 change moves logic from the `Xor(L|I)Node::Value` methods into the `add_ring` methods, and gives priority to constant-folding. A static method was separated out to facilitate direct unit-testing. It also (subjectively) simplified the  calculation of the upper bound and added an explanation of the reasoning behind it.
>> 
>> In addition to testing for constant folding over xor, IR tests were added to `XorINodeIdealizationTests` and `XorLNodeIdealizationTests` to cover these related items:
>>  - Bounds optimization of xor
>>  - A check for  `x ^ x = 0`
>>  - Explicit testing of xor over booleans.
>> 
>> Also `test_xor_node.cpp` was added to more extensively test the correctness of the bounds optimization. It exhaustively tests ranges of 4-bit numbers as well as at the high and low end of the affected types.
>> 
>> ---------
>> ### Progress
>> - [x] Change must not contain extraneous whitespace
>> - [x] Commit message must refer to an issue
>> - [ ] Change must be properly reviewed (2 reviews required, with at least 2 [Reviewers](https://openjdk.org/bylaws#reviewer))
>> 
>> 
>> 
>> ### Reviewers
>>  * [Quan Anh Mai](https://openjdk.org/census#qamai) (@merykitty - Committer) 🔄 Re-review required (review applies to [cf779497](https://git.openjdk.org/jdk/pull/23089/files/cf77949776f7a4601268c7291a5743c2eb164186))
>> 
>> ### Reviewing
>> <details><summary>Using <code>git</code></summary>
>> 
>> Checkout this PR locally: \
>> `$ git fetch https://git.openjdk.org/jdk.git pull/23089/head:pull/23089` \
>> `$ git checkout pull/23089`
>> 
>> Update a local copy of the PR: \
>> `$ git checkout pull/23089` \
>> `$ git pull https://git.openjdk.org/jdk.git pull/23089/head`
>> 
>> </details>
>> <details><summary>Using Skara CLI tools</summary>
>> 
>> Checkout this PR locally: \
>> `$ git pr checkout 23089`
>> 
>> View PR using the GUI difftool: \
>> `$ git pr show -t 23089`
>> 
>> </details>
>> <details><summary>Using diff file</summary>
>> 
>> Download this PR as a diff file: \
>> <a href="https://git.openjdk.org/jdk/pull/23089.diff">https://git.openjdk.org/jdk/pull/23089.diff</a>
>> 
>> </details>
>> <details><summary>Using Webrev</summary>
>> 
>> [Link to Webrev Comment](https://git.openjdk.org/jdk/pull/23089#issuecomment-25939...
>
> Johannes Graham has updated the pull request incrementally with one additional commit since the last revision:
> 
>   update tests

Thanks for the ping! Nice work, thanks for the improvements. I have a few more suggestions.

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

> 1077:       julong max = calc_xor_upper_bound_of_non_neg<jlong, julong>(r0->_hi, r1->_hi);
> 1078:       return TypeLong::make(0, max, MAX2(r0->_widen, r1->_widen));
> 1079:   }

Suggestion:

  if (r0->_lo >= 0 && r1->_lo >= 0) {
      // Combine [0, lo_1] ^ [0, hi_1] -> [0, max]
      julong max = calc_xor_upper_bound_of_non_neg<jlong, julong>(r0->_hi, r1->_hi);
      return TypeLong::make(0, max, MAX2(r0->_widen, r1->_widen));
  }

Some comment like this would be nice, it matches the `Constant fold` comment above.

test/hotspot/jtreg/compiler/c2/irTests/XorINodeIdealizationTests.java line 236:

> 234:     public int testConstXor() {
> 235:         return CONST_1 ^ CONST_2;
> 236:     }

Nice, that's a test for constant folding with random constants.

test/hotspot/jtreg/compiler/c2/irTests/XorINodeIdealizationTests.java line 334:

> 332:         var xor = (x & 0b111) ^ (y & 0b100);
> 333:         return xor < 0b1000;
> 334:     }

These are nice simple examples, and we should keep them. But I'm missing these cases with randomization.

`calc_xor_upper_bound_of_non_neg` basically has two input ranges `[0, hi_0]` and `[0, hi_1]`, and gives us a new range `[0,max]`.

You could produce random input ranges like this:

public void test(int x) {
    int lo_x = con1;
    int hi_x = con2;
    x = x < lo_x ? lo_x : (x > hi_x ? hi_x : x);
    // x clamped to [lo_x, hi_x]
    int lo_y = con3;
    int hi_y = con4;
    y = y < lo_y ? lo_y : (y > hi_y ? hi_y : y);
    // y clamped to [lo_y, hi_y]
    int z = x ^ y;
    // This should now have a new range, possibly some [0, max]
    // Now let's test the range with some random if branches.
    int sum = 0;
    if (z > somecon1) { sum += 1; }
    if (z > somecon2) { sum += 2; }
    if (z > somecon3) { sum += 4; }
    // maybe add a few more...
    if (z > someconi) { sum += pow(2,i); }
    return sum;
}

The `sum` at the end gives you a summary over all the checks. If one wrongly constant folds, you'll be missing one of the power of 2 contributions to it, or have it wrongly added.
Now you do this with an `x` and a `y`

Maybe that's a little over-engineered, but it would target the `calc_xor_upper_bound_of_non_neg` logic really well.
What do you think?

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

PR Review: https://git.openjdk.org/jdk/pull/23089#pullrequestreview-2639832127
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1969162329
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1969127608
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1969157206