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

[kuaiwei]

On Mon, 24 Mar 2025 06:49:31 GMT, kuaiwei <duke at openjdk.org> wrote:

>> @kuaiwei Just ping me when you would like me to re-review :)
>
> 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.

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.

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

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