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

[Roland Westrelin]

On Tue, 27 May 2025 08:11:27 GMT, Galder Zamarreño <galder 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
>>  ...
>
> test/hotspot/jtreg/compiler/c2/TestMismatchedAddPAfterMaxUnroll.java line 74:
> 
>> 72:         }
>> 73:         if (flag) {
>> 74:             if (flag2) {
> 
> It looks a bit odd to have this if statement and the flag one. One would expect these to be dead code eliminated? Or does the DCE only happen after the problematic C2 crash? Might be useful to have some comment explaining the rationale for these if statements.

What happens is that if a branch is never taken at runtime (as is the case here for `if (flag2) {` here), that's captured by profile data. If c2 sees a never taken branch, it doesn't parse it. So it is compiled to:

if (flags2) {
  uncommon_trap(unstable_if);
}

As a result, c2 never sees the branch is empty and the while statement is useless. It's a missed optimization opportunity but I find it useful for test cases and use that pattern.

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

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