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

[Quan Anh Mai]

On Thu, 13 Feb 2025 08:54:51 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Johannes Graham has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   formatting, remove commented tests
>
> 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`?

I think the name is good enough, calculate the max of a xor is a pretty self-explanatory name. You just need a better description for the method. Suggestion:

    Given 2 non-negative values in the ranges [0, hi_0] and [0, hi_1], respectively. The bitwise xor of these values should also be non-negative. This method calculates its maximum.

> 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?

`calc_xor_max` is a static method in the cpp file and thus it is not preferrable to import it directly into the test file.

> 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.

I think the comment should be removed, the specs and implementation of `calc_xor_max` should be referred to when trying to understand this piece of code.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1954132494
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1954135679
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1954143818