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

[Emanuel Peter]

On Tue, 2 Dec 2025 09:49:29 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 14 additional commits since the last revision:
> 
>  - more
>  - review
>  - Merge branch 'master' into JDK-8351889
>  - exp
>  - Merge branch 'master' into JDK-8351889
>  - verif
>  - Merge branch 'master' into JDK-8351889
>  - test seed
>  - more
>  - Merge branch 'master' into JDK-8351889
>  - ... and 4 more: https://git.openjdk.org/jdk/compare/d6d17aab...15c17bb1

I think I'm on board with the solution now. It is probably best to do it during IGVN. I have a few more suggestions below :)

src/hotspot/share/opto/cfgnode.cpp line 2171:

> 2169:       !wait_for_region_igvn(phase)) {
> 2170:     // If one of the inputs is a cast that has yet to be processed by igvn, delay processing of this node to give the
> 2171:     // inputs a chance to optimize and possibly end up with identical inputs.

I think we should have more detail here. Why is this a good idea? Is this an optimization? Or is it for correctness?

I think you should say something about possibly having multiple cast nodes that could be commoned, and then they would keep their ctrl. But if we uncast, then we lose the info about the ctrl, and below we insert a new cast with a different (later) ctrl. This has two downsides:
- The ctrl is later than necessary: suboptimal
- If we have 3 or more copies of casts with the same ctrl, and now we remove two and create a new one with a different ctrl, then the remaining old and the new cast cannot common because they have different ctrl.
  - this suboptimal
  - this also creates issues along AddP paths: it can be that at some AddP we get one cast and at another AddP a different cast. They all come from the same original base address, just casted differently. But it makes it difficult to check consistency, and asserts fail.

This is not very concise yet, you can probably formulate it in a better way ;)

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

> 2074:     if (addp->in(AddPNode::Base) == n->in(AddPNode::Base)) {
> 2075:       return false;
> 2076:     }

Suggestion:

    if (!addp->is_AddP() ||
        addp->in(AddPNode::Base)->is_top() ||
        addp->in(AddPNode::Base) == n->in(AddPNode::Base)) {
      return false;
    }

test/hotspot/jtreg/compiler/c2/TestMismatchedAddPAfterMaxUnroll.java line 35:

> 33:  *                   -XX:+StressIGVN TestMismatchedAddPAfterMaxUnroll
> 34:  * @run main/othervm TestMismatchedAddPAfterMaxUnroll
> 35:  */

What about a run with our new fancy flag `-XX:VerifyIterativeGVN=10000`?

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

PR Review: https://git.openjdk.org/jdk/pull/25386#pullrequestreview-3533246147
PR Review Comment: https://git.openjdk.org/jdk/pull/25386#discussion_r2583686701
PR Review Comment: https://git.openjdk.org/jdk/pull/25386#discussion_r2583690118
PR Review Comment: https://git.openjdk.org/jdk/pull/25386#discussion_r2583697429