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

[Vladimir Ivanov]

On Tue, 1 Apr 2025 02:44:03 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.
>
> Johannes Graham has updated the pull request incrementally with one additional commit since the last revision:
> 
>   remove unused methods

Overall, looks good.

Some minor comments follow.

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

> 1010: 
> 1011:   if (r0->is_con() && r1->is_con()) {
> 1012:     // Constant fold: (c1 ^ c2) -> c3

A bit confusing. The comment mentions `c1` and `c2` while the code operate on `t0`/`r0` and `t1`/`r1`.

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

> 1017: 
> 1018:   if (r0->_lo >= 0 && r1->_lo >= 0) {
> 1019:       // Combine [0, lo_1] ^ [0, hi_1] -> [0, max]

What does this comment refer to? It mentions `lo_1` and `hi_1` while `r0->_hi` and `r1->_hi` are passed into `xor_upper_bound_for_ranges`. 

Also, I'd avoid naming it`max`: it sort of hints to `max_jint`, but in reality it represents the upper bound of the operation. Why not `upper`/`upper_bound` instead?

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

PR Review: https://git.openjdk.org/jdk/pull/23089#pullrequestreview-2733792136
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r2023525192
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r2023523965