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

[Emanuel Peter]

On Mon, 24 Mar 2025 08:30:11 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

>> The [array fill optimization](https://github.com/openjdk/jdk/blob/1d147ccb4cfcb1da23664ac941e56ac542a7ac61/src/hotspot/share/opto/loopTransform.cpp#L3533) replaces simple innermost loops that fill an array with copies of the same primitive value:
>> 
>> 
>> for (int i = 0; i < array.length; i++) {
>>     array[i] = 0;
>> }
>> 
>> with a call to an [array filling intrinsic](https://github.com/openjdk/jdk/blob/1d147ccb4cfcb1da23664ac941e56ac542a7ac61/src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp#L1665) that is specialized for the array element type:
>> 
>> 
>> arrayof_jint_fill(array, 0, array.length)
>> 
>> 
>> The optimization retrieves the (basic) array element type from calling `MemNode::memory_type()` on the original filling store. This is incorrect for stores of `short` values, since these are represented by `StoreC` nodes [whose `memory_type()` is `T_CHAR`](https://github.com/openjdk/jdk/blob/1fe45265e446eeca5dc496085928ce20863a3172/src/hotspot/share/opto/memnode.hpp#L680). As a result, the optimization wrongly assigns the address type `char[]` to `short` array fill loops. This can cause miscompilations due to missing anti-dependences, see the [issue description for further detail](https://bugs.openjdk.org/projects/JDK/issues/JDK-8351468).
>> 
>> This changeset proposes retrieving the (basic) array element type from the store address type instead. This ensures that the accurate address type is assigned to the intrinsic call, preventing missed anti-dependences and other potential issues caused by mismatching types. Additionally, the changeset makes it easier to reason about correctness by explicitly disabling the optimization for mismatched stores (where the type of the value to be stored differs from the element type of the destination array). Such stores were not optimized before, but only due to pattern matching limitations.
>> 
>> Assuming mismatched stores are discarded (as proposed here), an alternative solution would be to define a StoreS node returning the appropriate `memory_type()`. This could be desirable even as a complement to this fix, to prevent similar bugs in the future. I propose to investigate the introduction of a StoreS node in a separate RFE, because it is a much larger and more intrusive changeset, and go with this minimal, local, and non-intrusive fix for backportability.
>> 
>> **Testing:** tier1-5, stress testing (linux-x64, windows-x64, macosx-x64, linux-aarch64, macosx-aarch64).
>
> Roberto Castañeda Lozano has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Relax test cases

@robcasloz Thanks for the changes!

Some thoughts about future work on intrinsic fill.
- It would be nice to enable mismatched cases.
- And it would be nice to enable not just arrays, but also native memory. That would be especially good for MemorySegments. But not sure how easy this change would be.

src/hotspot/share/opto/loopTransform.cpp line 3579:

> 3577:   if (msg == nullptr && store->as_Mem()->is_mismatched_access()) {
> 3578:     msg = "mismatched store";
> 3579:   }

What effect does this have?

Ah, it seems to have to do with these comments in your PR:
`Additionally, the changeset makes it easier to reason about correctness by explicitly disabling the optimization for mismatched stores (where the type of the value to be stored differs from the element type of the destination array). Such stores were not optimized before, but only due to pattern matching limitations.`

It may be good to leave additional comments in the code here, saying that this is a limitation, and maybe improved in the future. Up to you.

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

Marked as reviewed by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24005#pullrequestreview-2709715800
PR Review Comment: https://git.openjdk.org/jdk/pull/24005#discussion_r2009796837