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

[Emanuel Peter]

On Thu, 2 Oct 2025 09:29:34 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> What if we just relax the assert?  I failed to figure out what this assert is protecting us from by looking at the code.  So what happens in a product build or when this assert is commented out?
>
>> What if we just relax the assert? I failed to figure out what this assert is protecting us from by looking at the code. So what happens in a product build or when this assert is commented out?
> 
> For this particular test case, nothing. The assert is right before the cast nodes are removed, anyway. Once they are removed, the `AddP` in the chain all have the same base input.
> The risk, I think, is if some code that transforms a chain of `AddP`s (some time before the assert) wrongly assume they all have the same base.  It's also easier to write such a transformation if it's an invariant that a chain of `AddP`s have the same base (it's one less thing to worry about).

@rwestrel 
> Then Phi#514 which has 2 CastPPs as input with identical inputs is
transformed into another CastPP at the Phi constrol with the data
control of the CastPP as input. PhiNode::unique_input() with
uncast = true is where that happens. That's where things go wrong I
think.

Right, this is where we go from 2->3 `CastPP`. Every additional `CastPP` with the same input seems to be a liability.

> The 2 CastPPs have the same data input but not same control and igvn can't common them.

Do you know why we insert a new `CastPP` there, and why it is put not at the ctrl of the CastPP, but of the phi?
I suppose the ctrl of the phi is correct, but we do lose information there, and that later prevents the `CastPP` to common.

> The fix I propose is to delay the call to PhiNode::unique_input()
with uncast = true if the Phi's inputs are cast nodes and have yet
to be processed by igvn. This causes identical CastPPs to common and
then only the Phi has 2 identical inputs is transformed to that
input (rather than have a new CastPPs be created at a different
control).

Ok, so in this case, we prevent the phi from looking through the two CastPP and creating a new third one, because that would create a third CastPP with a new ctrl, that we cannot fold away later. Rather, we hope that the two CastPP get commoned first.

Ok, it is starting to make sense to me now.

@rwestrel Does what I describe match what you tried to explain so far?

@rwestrel Do you think we could have the same assert also after every IGVN? That a chain of `AddP` all have the same base? I think that would be a nice addition to this fix here, and would strengthen the invariant.

This could be part of `VerifyIterativeGVN`.

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

PR Comment: https://git.openjdk.org/jdk/pull/25386#issuecomment-3528554194
PR Comment: https://git.openjdk.org/jdk/pull/25386#issuecomment-3528567651