RFR: 8345485: C2 MergeLoads: merge adjacent array/native memory loads into larger load [v12]

[kuaiwei]

On Thu, 17 Apr 2025 09:54:33 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> kuaiwei has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 14 commits:
>> 
>>  - Merge remote-tracking branch 'origin/master' into dev/merge_loads
>>  - Remove unused code
>>  - Move code to addnode.cpp and add more tests
>>  - Merge remote-tracking branch 'origin/master' into dev/merge_loads
>>  - Fix test
>>  - Add more tests
>>  - Enable StressIGVN and riscv platform
>>  - Change tests as review comments
>>  - Fix test failure and change for review comments
>>  - Revert extract value and add more tests
>>  - ... and 4 more: https://git.openjdk.org/jdk/compare/660b17a6...f6518b26
>
> src/hotspot/share/opto/addnode.cpp line 816:
> 
>> 814:   LoadNode* const    _load;
>> 815:   Node*     const _combine;
>> 816:   int       const   _shift;
> 
> Suggestion:
> 
>   jint       const   _shift;
> 
> I prefer using `jint` etc when we are talking about values that correlate to java types. With the C/C++ numerical types, there could always be issues on different platforms with different bit sizes.

Fixed

> src/hotspot/share/opto/addnode.cpp line 843:
> 
>> 841:   Node*     const    _combine;
>> 842:   MemoryAdjacentStatus _order;
>> 843:   bool _require_reverse_bytes;    // Do we need add a ReverseBytes for merged load
> 
> Suggestion:
> 
>   bool _require_reverse_bytes;    // Do we need to add a ReverseBytes for merged load

Fixed

> src/hotspot/share/opto/addnode.cpp line 857:
> 
>> 855: 
>> 856: private:
>> 857:   // Detect the embedding combine node is a candidate for merging loads
> 
> Suggestion:
> 
>   // Detect if the embedding combine node is a candidate for merging loads

Fixed

> src/hotspot/share/opto/addnode.cpp line 920:
> 
>> 918: 
>> 919: // Go through ConvI2L which is unique output of input node
>> 920: const Node* MergePrimitiveLoads::by_pass_i2l(const Node* n) {
> 
> Suggestion:
> 
> const Node* MergePrimitiveLoads::bypass_ConvI2L(const Node* n) {
> 
> I think `bypass` is a single verb, not two wordy ;)

Fixed

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24023#discussion_r2062835992
PR Review Comment: https://git.openjdk.org/jdk/pull/24023#discussion_r2062835815
PR Review Comment: https://git.openjdk.org/jdk/pull/24023#discussion_r2062835896
PR Review Comment: https://git.openjdk.org/jdk/pull/24023#discussion_r2062836374