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

[Roberto Castañeda Lozano]

On Fri, 5 Dec 2025 13:48:50 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 incrementally with one additional commit since the last revision:
> 
>   review

Thanks for the thorough analysis and fix, Roland! I agree this is the best way to go at the moment.

The invariant that AddP chains share the exact same base address node seems useful, as you mention above, to reduce the number of cases to think about. However, if we find in the future that this invariant becomes too difficult to maintain, we might want to consider relaxing it to "AddP chains have the same *uncast* base address". I guess this is the relaxation that @dean-long suggested above, and I believe it would preserve the spirit of the original assertion added to `Compile::final_graph_reshaping_main_switch()` by @rose00 many years ago when implementing the CastX2P/CastP2X nodes (perhaps to verify chains of AddP nodes induced by `CastX2PNode::Ideal` transformations?).

I'm running some internal testing, will come back with results.

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

> 2062: }
> 2063: 
> 2064: // Some other verifications that are no specific to a particular transformation

Suggestion:

// Some other verifications that are not specific to a particular transformation.

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

> 2063: 
> 2064: // Some other verifications that are no specific to a particular transformation
> 2065: bool PhaseIterGVN::verify_node_invariants_for(Node* n) {

Suggestion:

bool PhaseIterGVN::verify_node_invariants_for(const Node* n) {

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

> 2068:     if (addp->is_AddP() &&
> 2069:         !addp->in(AddPNode::Base)->is_top() &&
> 2070:         addp->in(AddPNode::Base) != n->in(AddPNode::Base)) {

Any way we could avoid this code duplication with the same check in `Compile::final_graph_reshaping_main_switch`?

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

> 494:   bool verify_Ideal_for(Node* n, bool can_reshape);
> 495:   bool verify_Identity_for(Node* n);
> 496:   bool verify_node_invariants_for(Node* n);

Suggestion:

  bool verify_node_invariants_for(const Node* n);

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

PR Review: https://git.openjdk.org/jdk/pull/25386#pullrequestreview-3567683298
PR Review Comment: https://git.openjdk.org/jdk/pull/25386#discussion_r2610782593
PR Review Comment: https://git.openjdk.org/jdk/pull/25386#discussion_r2610784474
PR Review Comment: https://git.openjdk.org/jdk/pull/25386#discussion_r2610807655
PR Review Comment: https://git.openjdk.org/jdk/pull/25386#discussion_r2610790721