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

[Emanuel Peter]

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?

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

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