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)

[Christian Hagedorn]

On Mon, 7 Jun 2021 11:19:43 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.

Looks reasonable to me!

src/hotspot/share/opto/castnode.hpp line 39:

> 37:     RegularDependency, // if cast doesn't improve input type, cast can be removed
> 38:     StrongDependency,  // leave cast in even if _type doesn't improve input type, can be replaced by stricter dominating cast if one exist
> 39:     VeryStrongDependency // leave cast in unconditionally

How about directly using something like `UnconditionalDependency` instead of `VeryStrongDepedendency` to better distinguish it?

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

> 1504:           register_new_node(x, x_ctrl);
> 1505: 
> 1506:           if (x->in(0) == NULL && !x->is_DecodeNarrowPtr() && (!x->is_AddP() || !x->in(AddPNode::Address)->is_AddP())) {

Can you add a comment here about this special case?

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

Marked as reviewed by chagedorn (Reviewer).

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