RFR: 8318446: C2: optimize stores into primitive arrays by combining values into larger store [v17]

[Roland Westrelin]

On Thu, 7 Mar 2024 07:45:13 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> src/hotspot/share/opto/memnode.cpp line 2971:
>> 
>>> 2969: // The goal is to check if two such ArrayPointers are adjacent for a load or store.
>>> 2970: //
>>> 2971: // Note: we accumulate all constant offsets into constant_offset, even the int constant behind
>> 
>> Is this really needed? For the patterns of interest, aren't the constant pushed down the chain of `AddP` nodes so the address is `(AddP base (AddP ...) constant)`?
>
> No, they are not pushed down.
> Consider the access on an int array:
> `a[invar + 1]` -> `adr = base + ARRAY_INT_BASE_OFFSET + 4 * ConvI2L(invar + 1)`
> We cannot just push the constant `1` out of the `ConvI2L`, after all `invar + 1` could overflow in the int domain ;)

That's not quite right, I think. For instance, in this method:

   private static int test(int[] array, int i) {
        return array[i + 1];
    }

the final IR will have the  `(AddP base (AddP ...) constant)` because `ConvI2LNode::Ideal` does more than checking for overflow. The actual transformation to that final shape must be delayed until after the CastII nodes are removed though. Why that's the case is puzzling actually because `CastIINode::Ideal()` has logic to push the AddI thru the `CastII` but it's disabled for range check `CastII` nodes. I noticed this while working on 8324517. My recollection was that `ConvI2LNode::Ideal` would push thru both the `CastII` and `ConvI2L` in one go so  I wonder if it got broken at some point.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16245#discussion_r1516290785