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

[Zoltán Majó]

Hi Martin,


On 04/27/2016 10:40 AM, Doerr, Martin wrote:
> 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/

looks good to me!

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

Sure. I can push the change once Vladimir has also agreed to it.

Best regards,


Zoltan

>
> 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
>>
>>