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

[Zoltán Majó]

The latest patch looks good to me as well. I'll push it today.

Thank you!

Best regards,


Zoltan

On 04/27/2016 04:54 PM, Vladimir Kozlov wrote:
> This looks good.
>
> Thanks,
> Vladimir
>
> On 4/27/16 1: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/ 
>>
>>
>> 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
>>>
>>>