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

[Emanuel Peter]

On Mon, 24 Mar 2025 11:00:43 GMT, kuaiwei <duke at openjdk.org> wrote:

>> Hi, @eme64 , I changed as comments and merge with master branch. It look no issue in testing. Could you help check it again? Thanks.
>
>> @kuaiwei I have not yet had the time to read through the PR, but I would like to talk about `LoadNode::Ideal`. The idea with `Ideal` in general, is that you replace one node with another. After `Ideal` returns, all usages of the old node now take the new node instead.
>> 
>> You copied the structure from my MergeStores implementation in `StoreNode::Idea`. There it made sense to replace `StoreB` nodes that have a memory output with `LoadI` nodes, which also have memory output.
>> 
>> But it does not make sense to replace a `LoadB` that has a byte/int output with a `LoadL` that has a long output for example.
>> 
>> I think your implementation should go into `OrINode`, and match the expression up from there. Because we want to replace the old `OrI` with the new `LoadL`.
>> 
>> Another question: Do you have some tests where some of the nodes in the `load/shift/or` expression have other uses? Imagine this:
>> 
>> ```
>> l0 = a[0];
>> l1 = a[1];
>> l2 = a[2];
>> l3 = a[3];
>> l = <merge l0...l1 via shift and or>;
>> now use l1 for something else as well
>> ```
>> 
>> What happens now? Do you check that we only use the old `LoadB` in the expression we are replacing?
> 
> Hi @eme64 , I understand your concern. In this patch , I check the usage of all `loadB` nodes and only allow they have only single usage into `OrNode`, I also check the `OrNode` as well.  So I think it will not cause the trouble.
> 
> 
> l0 = a[0];
> l1 = a[1];
> l2 = a[2];
> l3 = a[3];
> l = <merge l0...l1 via shift and or>;
> now use l1 for something else as well
> 
> For this case, because l1 has other usage, all these loads will not be merged.
> 
> In my previous patch, I tried to extract value from merged `LoadNode` if origin `loadB` has other usage, such as used by uncommon trap. You can find them in https://github.com/openjdk/jdk/pull/24023/commits/b621db1cf0c17885516254a2af4b5df43e06c098 and search MergePrimitiveLoads::extract_value_for_uncommon_trap . But in my test with jtreg tier1, it never hit a case which replaced `LoadB` used by uncommon trap, I think range check smearing remove all the uncommon trap usages. So I revert it to make code simple. In my opinion, the extract_value function can be used as a general solution for other usages. But we may need a cost model to evaluate cost of new instructions which used for extracting and benefit of merged load. To simplify, I choose to check usage strictly.

@kuaiwei Thanks for your response!

What about these two things I brought up?

> Do you have some tests where some of the nodes in the load/shift/or expression have other uses?

It would be good to have these tests, even if we think your code is correct. It is good to verify it with tests. And someone in the future might break it.

> I think your implementation should go into OrINode, and match the expression up from there. Because we want to replace the old OrI with the new LoadL.

This is really the pattern we use in `Idea`. We replace the node at the bottom of an expression with a new node (or new expression).

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

PR Comment: https://git.openjdk.org/jdk/pull/24023#issuecomment-2747835311