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

[Emanuel Peter]

On Mon, 27 Oct 2025 10:40:18 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

> **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, 16777216
> 
> We can see the `zero`, followed by the `ConL`, and then 3 `ConI`.
> 
> This sequence is then serialized into a stream, and stored in the `nmethod`, as part of the compilation.
> 
> 3) Once we deopt,...

src/hotspot/share/opto/macro.cpp line 874:

> 872:       assert(false, "field_val does not fit field_type");
> 873:     }
> 874: #endif

I'm not yet happy with this assert. It is not super easy to get it right, but currently it is a bit weak.

Do reviewers have any good ideas here?

test/hotspot/jtreg/compiler/c2/TestMergeStoresAndAllocationElimination.java line 40:

> 38:  */
> 39: 
> 40: public class TestMergeStoresAndAllocationElimination {

This is the reproducer for the wrong result.

test/hotspot/jtreg/compiler/escapeAnalysis/TestRematerializeObjects.java line 47:

> 45: /**
> 46:  * More complicated test cases can be found in {@link TestRematerializeObjectsFuzzing}.
> 47:  */

Suggestion:

test/hotspot/jtreg/compiler/escapeAnalysis/TestRematerializeObjects.java line 48:

> 46:  * More complicated test cases can be found in {@link TestRematerializeObjectsFuzzing}.
> 47:  */
> 48: public class TestRematerializeObjects {

These tests would not reproduce, but at least they let me check for the success of MergeStores and allocation elimination, and we get some examples that run through `PhaseMacroExpand::create_scalarized_object_description`.

test/hotspot/jtreg/compiler/escapeAnalysis/TestRematerializeObjects.java line 52:

> 50:     public static void main(String[] args) {
> 51:         TestFramework framework = new TestFramework(TestRematerializeObjects.class);
> 52:         //framework.addFlags("-XX:-TieredCompilation", "-Xbatch", "-XX:-CICompileOSR");

Suggestion:

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27997#discussion_r2472111485
PR Review Comment: https://git.openjdk.org/jdk/pull/27997#discussion_r2472116101
PR Review Comment: https://git.openjdk.org/jdk/pull/27997#discussion_r2472117508
PR Review Comment: https://git.openjdk.org/jdk/pull/27997#discussion_r2472121111
PR Review Comment: https://git.openjdk.org/jdk/pull/27997#discussion_r2472118058