RFR: 8366490: C2 SuperWord: wrong result because CastP2X is missing ctrl and floats over SafePoint creating stale oops [v2]

[Manuel Hässig]

On Tue, 2 Sep 2025 12:58:37 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> **Analysis**
>> 
>> A `CastP2X` without ctrl can float. If it floats over a `SafePoint` (or call), we may GC and move the oop. But the `CastP2X` value does not end up on the oop-map, and so the pointer is stale (old).
>> 
>> With `StressGCM`, the aliasing runtime check has one `CastP2X` that floats over the SafePoint, and another that stays after the SafePoint. Both read the oop of the same array, so instead of getting the same address, we now get the old and the new oop. And so the aliasing runtime check passes (thinks there is no aliasing), even though there is aliasing. We end up vectorizing, which reorders the loads/stores and would only be safe if there is no aliasing.
>> 
>> **Fix:** add control to the `CastP2X` so that it cannot float too far.
>> 
>> **Details**
>> 
>> 
>> rbp = Allcoate array
>> spill <- rbp + 0x20
>> 
>> call to allocateArrays
>> -> allocates a lot, and triggers GC. That moves the allocated array behind rbp
>> -> rbp is oop-mapped, so it is updated automatically to the new oop
>> -> spill value remains based on the old oop
>> 
>> We now compute the aliasing runtime check:
>> -> one side of the comparison is computed from rbp (new oop)
>> -> the other side is computed from the the spill value (old oop)
>> -> the cmp returns a nonsensical value, and we take the wrong branch
>> -> vectorize even though we have aliasing!
>
> Emanuel Peter has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - fix test requires
>  - Apply suggestions from code review
>    
>    Co-authored-by: Tobias Hartmann <tobias.hartmann at oracle.com>

Thank you for the fix and the easy to follow analysis, @eme64. I just have a few minor comments. Otherwise, this looks good.

src/hotspot/share/opto/vectorization.cpp line 1128:

> 1126:       Node* variable = (s.variable() == iv) ? iv_value : s.variable();
> 1127:       if (variable->bottom_type()->isa_ptr() != nullptr) {
> 1128:         // Make sure that ctrl is late enough, so that we do not

Suggestion:

        // Use a ctrl that is late enough, so that we do not

At first, I read this as "we need to make sure here that the `ctrl` is late enough` when we really use a `ctrl` that is passed and we cannot really affect its place anymore. But feel free to ignore.

test/hotspot/jtreg/compiler/loopopts/superword/TestAliasingCastP2XCtrl.java line 31:

> 29:  *          from floating over a SafePoint that could move the oop,
> 30:  *          and render the cast value stale.
> 31:  *

Suggestion:


Nit: superfluous empty line

test/hotspot/jtreg/compiler/loopopts/superword/TestAliasingCastP2XCtrl.java line 71:

> 69:             int[] a = new int[N];
> 70:         }
> 71:         // Makes GC more likely.

No clue if this is the right use case, but maybe this would be a good use of `-XX:+GCALot`?

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

Marked as reviewed by mhaessig (Committer).

PR Review: https://git.openjdk.org/jdk/pull/27045#pullrequestreview-3176427125
PR Review Comment: https://git.openjdk.org/jdk/pull/27045#discussion_r2316012817
PR Review Comment: https://git.openjdk.org/jdk/pull/27045#discussion_r2316005451
PR Review Comment: https://git.openjdk.org/jdk/pull/27045#discussion_r2316002575