RFR: 8267988: C2: assert(!addp->is_AddP() || addp->in(AddPNode::Base)->is_top() || addp->in(AddPNode::Base) == n->in(AddPNode::Base)) failed: Base pointers must match (addp 1301) [v3]

[Vladimir Ivanov]

On Thu, 10 Jun 2021 09:27:27 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> The failure occurs because, in a loop body, two AddP nodes are
>> chained:
>> (AddPNode base (AddPNode base ..) ..)
>> 
>> One can be sunk out of loop but not the other which results in:
>> (AddPNode (CastPP base) (AddPNode base ..) ..)
>> 
>> And the 2 AddP nodes no longer have the same base.
>> 
>> The fix I propose is to make sure sinking nodes out of loop preserve
>> the invariant that 2 chained AddP nodes have the same base. It's not
>> straightfoward given the 2 AddP nodes are sunk one after the other and
>> when sinking the first one, it's hard to tell whether the second one
>> can be sunk as well.
>> 
>> With the fix:
>> 
>> - when the first AddP is sunk, no CastPP node is added to pin the AddP
>>   out of the loop so if sinking the second AddP fails, the 2 AddP
>>   nodes still have the same base.
>> 
>> - when the second AddP is sunk, a CastPP node is created and wired so
>>   both AddP base is updated to the new CastPP.
>> 
>> This change also addresses another unrelated minor issue. If, for
>> instance the loop has a null check, the loop body already includes a
>> CastPP. So if that CastPP is used by an AddP that is sunk, we end up
>> with:
>> 
>> (AddPNode (CastPP (CastPP...
>> 
>> Then code in ConstraintCastNode::dominating_cast(), causes the CastPP
>> added to pin the AddP out of loop to be replaced by the CastPP in the
>> loop so sinking the AddP has no effect. To prevent that, I tweaked the
>> way a ConstrainedCast is marked as carrying a dependency so
>> ConstraintCastNode::dominating_cast() doesn't trigger.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
> 
>  - review
>  - merge
>  - review
>  - review
>  - fix

src/hotspot/share/opto/loopopts.cpp line 1510:

> 1508:           // is only added once both AddP nodes are sunk (see special code for AddP after the cast is created).
> 1509:           if (x->in(0) == NULL && !x->is_DecodeNarrowPtr() &&
> 1510:               !(x->is_AddP() && x->in(AddPNode::Address)->is_AddP() && x->in(AddPNode::Address)->in(AddPNode::Base) == x->in(AddPNode::Base))) {

I read the last check as "not a case of chained AddP with different bases". 
Then the comparison should be `!=`, shouldn't it?

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

PR: https://git.openjdk.java.net/jdk/pull/4387