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

[Roland Westrelin]

On Fri, 21 Nov 2025 10:13:31 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> @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?
>
>> 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.
> 
> When the `Phi` is removed because all of its inputs are the same once uncasted, there is a risk of losing a dependency. To prevent that, a `CastPP` is inserted. All we know is that some casts along some inputs of the `Phi` may carry a dependency that we don't want to loose. The only possible control for the `CastPP` then is the one of the `Phi`.

> @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`.

That would make sense I think.

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

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