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

[Emanuel Peter]

On Thu, 13 Feb 2025 09:26:42 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

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

I suppose this is more of a cosmetic concern, not as important. Up to you what you want to do @j3graham .

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

I think a better name like `calculate_upper_bound_of_xor_with_non_negative_inputs` would help a lot, and make some comments redundant.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1954181880
PR Review Comment: https://git.openjdk.org/jdk/pull/23089#discussion_r1954183571