RFR: 8351468: C2: array fill optimization assigns wrong type to intrinsic call

[Roberto Castañeda Lozano]

On Fri, 14 Mar 2025 09:54:53 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

>> @robcasloz I disagree, I would expect the `memory_type` of a `StoreC` into a `long[]` to be something that means "a part of a `long[]`", which should be `T_LONG` if the store is guaranteed to be enclosed in a single `long`, or `T_VOID` otherwise. While we are trying to store 2 bytes into the memory, the thing in the memory is neither a `short` nor a `char`.
>
>> I would expect the `memory_type` of a `StoreC` into a `long[]` to be something that means "a part of a `long[]`"
> 
> If that was the intended meaning of `MemNode::memory_type()`, wouldn't the function be redundant, because we can retrieve that information from `MemNode::adr_type()` already?

> @robcasloz Yes that's right. Then `MemNode::memory_type()` does not refer to the thing in memory at all, but the thing that is about to interact with the memory.

Yes, that matches my understanding.

> * We should rename it to `MemNode::value_type()` or `MemNode::value_basic_type()`

I agree, it would be good to do this (in a follow-up RFE). I like `MemNode::value_basic_type()` best.

> It is simply incorrect to use it to reason about the thing in the memory in this problem, and using adr_type is the correct fix.
>
> To be clear, I don't think having `StoreSNode` would solve any issue. I can `StoreS` into a `char[]`, and `StoreC` into a `short[]` and we are back at the same issue.

I agree that using `adr_type()` (the solution proposed in this changeset) seems more robust.

The alternative of using `memory_type()` and introducing a `StoreS` node assumes for correctness that the array fill optimization does not succeed for mismatched stores such as those you mention (e.g. `StoreS` into a `char[]`). If it did, I agree using `memory_type()` would be incorrect even after introducing a `StoreS` node. But so far, I haven't found any counterexample, i.e. any way to produce an array-filling loop with such a mismatched store that would be accepted by the array fill optimization. My attempts include using memory segments and Unsafe. In all cases, the array fill analysis in `PhaseIdealLoop::match_fill_loop` fails to recognize the loops due to different address computation patterns. Do you have any other idea/suggestion to trigger the array fill optimization using mismatched array stores?

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

PR Comment: https://git.openjdk.org/jdk/pull/24005#issuecomment-2724980394