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

[kuaiwei]

On Thu, 17 Apr 2025 09:58:24 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 912:
> 
>> 910:     return false;
>> 911:   }
>> 912:   return true;
> 
> This feels inverted. I would do a switch case here anyway, and return `false` in the default case.
> 
> Hmm. LoadS is supposed to be implicitly sign extended. Can those upper sign bits not lead to issues when OR-ing without a mask?

The sign bit is checked in `MergePrimitiveLoad::collect_merge_list`. 
```c++
    // Check sign bit of load
    // For shifted value based on memory load, if it does not reach the sign bit of merged load,
    // the load must be an unsigned load
    if ((info->_shift + load->memory_size() * BitsPerByte) != (collected * load->memory_size() * BitsPerByte)) {
      if (!info->_load->is_unsigned()) {
        // no unsigned Load of LoadI, so LoadI can not be merged
        // we may check value, if it's greater than 0, it can be merged
        return;
      }
    }

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24023#discussion_r2052279994