RFR(S): 8154836: VM crash due to "Base pointers must match"

[Doerr, Martin]

Hi Vladimir and Zoltan,

thanks for reviewing.

I have made the requested changes:
- Removed the code which skips the transformation on non-X86 platforms in heap-based compressed oops mode. I think we can discuss that independently.
- Improved the assertion as suggested by Vladimir.

New webrev is here:
http://cr.openjdk.java.net/~mdoerr/8154836_final_graph_reshaping/webrev.01/

Zoltan, you have already offered to sponsor this change. Thanks.

Best regards,
Martin


-----Original Message-----
From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com] 
Sent: Mittwoch, 27. April 2016 03:47
To: Doerr, Martin <martin.doerr at sap.com>; Zoltán Majó <zoltan.majo at oracle.com>; hotspot-compiler-dev at openjdk.java.net compiler <hotspot-compiler-dev at openjdk.java.net>
Subject: Re: RFR(S): 8154836: VM crash due to "Base pointers must match"

Thank you, Martin

On 4/25/16 4:04 AM, Doerr, Martin wrote:
> Hi all,
>
> we have already seen such an assertion in final_graph_reshaping.
>
> We found 2 AddP nodes in a row. The ideal graph looked like this (simplified):
> N0 ConP
> N1 ConN
> N2 AddP(Base = N0, Address = N0)
> N3 AddP(Base = N0, Address = N2)
>
> Final graph reshaping visited N2 before N3 first and changed the graph:
> N0 ConP
> N1 ConN
> N4 DecodeN
> N2 AddP(Base = N4, Address = N4)
> N3 AddP(Base = N0, Address = N2)
>
> Afterwards, final graph reshaping visited N3 and ran into the assertion. The Base of N3 is unexpected.
>
> I made a change to reconnect N3's Base input to N4, too.
>
> Webrev is here:
> http://cr.openjdk.java.net/~mdoerr/8154836_final_graph_reshaping/webrev.00/

Fix looks good.

>
> In addition to fixing this problem, I added an assertion to check if there are more than 3 AddP nodes in a row.
> I wouldn't expect that to happen. Not sure if this assertion is desired.

We should not have 3rd AddP but I agree with assert. You should add additional check to the assert:

|| out_j->in(AddPNode::Base) != addp

>
> I made an additional change: I think the graph transformation doesn't make sense if decoding is expensive.
> Therefore, I skip it on non-X86 platforms when we're running in heap based compressed oops mode.
> (I believe X86 is the only platform which can match the decoding in the operand in this case.)

It is not related to the fix and should be done separately. Or don't do at all.
There is comment above the code which explains why it could beneficial on SPARC too:

2845       // On sparc loading 32-bits constant and decoding it have less
2846       // instructions (4) then load 64-bits constant (7).

Thanks,
Vladimir

>
> Please review. I will also need a sponsor, please.
>
> Best regards,
> Martin
>
>