RFR: 8353345: C2 asserts because maskShiftAmount modifies node without deleting the hash [v2]

[Marc Chevalier]

On Thu, 3 Apr 2025 05:09:29 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Marc Chevalier has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Fix spacing
>>  - Do not eagerly replace shift amounts in nested lshift
>
> src/hotspot/share/opto/mulnode.cpp line 953:
> 
>> 951: }
>> 952: 
>> 953: //=============================================================================
> 
> While at it, you can also remove this line which we no longer use today
> Suggestion:

Done.

> src/hotspot/share/opto/mulnode.cpp line 995:
> 
>> 993:       if (igvn != nullptr) {
>> 994:         igvn->rehash_node_delayed(shiftNode);
>> 995:       }
> 
> Do we still need this now? If we always call it with `shiftNode == this` then we already get the rehashing "for free" due to modifying `this` as part of `Ideal()`.

As discussed, do not remove hash (useless now), but enqueue in worklist.

> src/hotspot/share/opto/mulnode.cpp line 1007:
> 
>> 1005: //   outer_shift = (_ << rhs0)
>> 1006: // We are looking for the pattern:
>> 1007: //   outer_shift = ((X << rhs1) << rhs0)
> 
> Just an idea: To better keep track of what is the outer and inner rhs, we could use `rhs_inner` and `rhs_outer`.

good idea!

> src/hotspot/share/opto/mulnode.cpp line 1010:
> 
>> 1008: //   where rhs0 and rhs1 are constant
>> 1009: //   we denote inner_shift the nested expression (X << rhs1)
>> 1010: //   con0 = rhs1 % nbits and con0 = rhs1 % nbits
> 
> Probably copy-paste error, did you want to define `con1` here as well?

indeed. redone with new notations.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24355#discussion_r2026584728
PR Review Comment: https://git.openjdk.org/jdk/pull/24355#discussion_r2026585502
PR Review Comment: https://git.openjdk.org/jdk/pull/24355#discussion_r2026585778
PR Review Comment: https://git.openjdk.org/jdk/pull/24355#discussion_r2026586168