RFR: 8351889: C2 crash: assertion failed: Base pointers must match (addp 344) [v3]

[Emanuel Peter]

On Fri, 21 Nov 2025 11:19:51 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> The test case has an out of loop `Store` with an `AddP` address
>> expression that has other uses and is in the loop body. Schematically,
>> only showing the address subgraph and the bases for the `AddP`s:
>> 
>> 
>> Store#195 -> AddP#133 -> AddP#134 -> CastPP#110
>>                      -> CastPP#110
>> 
>> 
>> Both `AddP`s have the same base, a `CastPP` that's also in the loop
>> body.
>> 
>> That loop is a counted loop and only has 3 iterations so is fully
>> unrolled. First, one iteration is peeled:
>> 
>> 
>>                                 /-> CastPP#110
>> Store#195 -> Phi#360 -> AddP#133 -> AddP#134 -> CastPP#110
>>                     -> AddP#277 -> AddP#278 -> CastPP#283
>>                                 -> CastPP#283
>> 
>> 
>> 
>> The `AddP`s and `CastPP` are cloned (because in the loop body). As
>> part of peeling, `PhaseIdealLoop::peeled_dom_test_elim()` is
>> called. It finds the test that guards `CastPP#283` in the peeled
>> iteration dominates and replaces the test that guards `CastPP#110`
>> (the test in the peeled iteration is the clone of the test in the
>> loop). That causes `CastPP#110`'s control to be updated to that of the
>> test in the peeled iteration and to be yanked from the loop. So now
>> `CastPP#283` and `CastPP#110` have the same inputs.
>> 
>> Next unrolling happens:
>> 
>> 
>>                                            /-> CastPP#110
>>                                /-> AddP#400 -> AddP#401 -> CastPP#110
>> Store#195 -> Phi#360 -> Phi#477 -> AddP#133 -> AddP#134 -> CastPP#110
>>                   \                        -> CastPP#110
>>                    -> AddP#277 -> AddP#278 -> CastPP#283
>>                                -> CastPP#283
>> 
>> 
>> 
>> `AddP`s are cloned once more but not the `CastPP`s because they are
>> both in the peeled iteration now. A new `Phi` is added.
>> 
>> Next igvn runs. It's going to push the `AddP`s through the `Phi`s.
>> 
>> Through `Phi#477`:
>> 
>> 
>> 
>>                                 /-> CastPP#110
>> Store#195 -> Phi#360 -> AddP#510 -> Phi#509 -> AddP#401 -> CastPP#110
>>                   \                        -> AddP#134 -> CastPP#110
>>                    -> AddP#277 -> AddP#278 -> CastPP#283
>>                                -> CastPP#283
>> 
>> 
>> 
>> Through `Phi#360`:
>> 
>> 
>>                                            /-> AddP#134 -> CastPP#110
>>                                 /-> Phi#509 -> AddP#401 -> CastPP#110
>> Store#195 -> AddP#516 -> Phi#515 -> AddP#278 -> CastPP#283
>>                      -> Phi#514 -> CastPP#283
>>  ...
>
> Roland Westrelin 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 10 additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8351889
>  - verif
>  - Merge branch 'master' into JDK-8351889
>  - test seed
>  - more
>  - Merge branch 'master' into JDK-8351889
>  - Merge branch 'master' into JDK-8351889
>  - more
>  - test
>  - fix

src/hotspot/share/opto/phaseX.cpp line 2085:

> 2083:   }
> 2084:   return false;
> 2085: }

Why not call it `verify_node_invariants_for`?

You should also assert immediately. @benoitmaillard Is about to make that change for everything: https://github.com/openjdk/jdk/pull/28295

src/hotspot/share/opto/phaseX.hpp line 623:

> 621:     // '-XX:VerifyIterativeGVN=10000'
> 622:     return ((VerifyIterativeGVN % 100000) / 10000) == 1;
> 623:   }

You will need to add extra documentation to the flag. And also there is a test that uses the flag. You should adjust it to enable this bit as well.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25386#discussion_r2556012167
PR Review Comment: https://git.openjdk.org/jdk/pull/25386#discussion_r2555714627