RFR: 8370405: C2: mismatched store from MergeStores wrongly scalarized in allocation elimination [v2]

[Quan Anh Mai]

On Wed, 29 Oct 2025 17:07:41 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Note: @oliviermattmann found this bug with his whitebox fuzzer. See also https://github.com/openjdk/jdk/pull/27991
>> 
>> **Analysis**
>> We run Escape Analysis, and see that a local array allocation could possibly be removed, we only have matching `StoreI` to the `int[]`. But there is one `StoreI` that is still in a loop, and so we wait with the actual allocation removal until later, hoping it may go away, or drop out of the loop.
>> During loop opts, the `StoreI` drops out of the loop, now there should be nothing in the way of allocation removal.
>> But now we run `MergeStores`, and merge two of the `StoreI` into a mismatched `StoreL`.
>> 
>> Then, we eventually remove the allocation, but don't check again if any new mismatched store has appeared.
>> Instead of a `ConI`, we receive a `ConL`, for the first of the two merged `StoreI`. The second merged `StoreI` instead captures the state before the `StoreL`, and that is wrong.
>> 
>> **Solution**
>> We should have some assert, that checks that the captured `field_val` corresponds to the expected `field_type`.
>> 
>> But the real fix was suggested by @merykitty : apparently he just had a similar issue in Valhalla:
>> https://github.com/openjdk/valhalla/blame/60af17ff5995cfa5de075332355f7f475c163865/src/hotspot/share/opto/macro.cpp#L709-L713
>> (the idea is to bail out of the elimination if any of the found stores are mismatched.)
>> 
>> **Details**
>> 
>> How the bad sequence develops, and which components are involved.
>> 
>> 1) The `SafePoint` contains a `ConL` and 3 `ConI`. (Correct would have been 4 `ConI`)
>> 
>>   6  ConI  === 23  [[ 4 ]]  #int:16777216
>>   7  ConI  === 23  [[ 4 ]]  #int:256
>>   8  ConI  === 23  [[ 4 ]]  #int:1048576
>>   9  ConL  === 23  [[ 4 ]]  #long:68719476737
>>  54  DefinitionSpillCopy  === _ 27  [[ 16 12 4 ]]
>>   4  CallStaticJavaDirect  === 47 29 30 26 32 33 0 34 0 54 9 8 7 6  [[ 5 3 52 ]] Static wrapper for: uncommon_trap(reason='unstable_if' action='reinterpret' debug_id='0') # void ( int ) C=0.000100 Test::test @ bci:38 (line 21) reexecute !jvms: Test::test @ bci:38 (line 21)
>> 
>> 
>> 2) This is then encoded into an `ObjectValue`. A `Type::Long` / `ConL` is converted into a `[int=0, long=ConL]` pair, see:
>> https://github.com/openjdk/jdk/blob/da7121aff9eccb046b82a75093034f1cdbd9b9e4/src/hotspot/share/opto/output.cpp#L920-L925
>> If I understand it right, there zero is just a placeholder.
>> 
>> And so we get:
>> 
>> (rr) p sv->print_fields_on(tty)
>>         Fields: 0, 68719476737, 1048576, 256, 167772...
>
> Emanuel Peter has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 16 additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8370405-alloc-elimination-and-MergeStores
>  - only verify primitive types
>  - Apply suggestions from code review
>  - more assert adjustment
>  - ignore debug flag
>  - id for tests, and fix up the assert
>  - pass int for short slot
>  - another test
>  - improve test
>  - wip new IR test
>  - ... and 6 more: https://git.openjdk.org/jdk/compare/60350647...b6e032c2

Regardless, I think this patch makes sense. Bailing out of scalar elimination when we are doing it is better than when we are running EA, and we should generally try to do it if we can.

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

PR Comment: https://git.openjdk.org/jdk/pull/27997#issuecomment-3466382031