RFR(XS): 8168662: Intrinsic support for event based tracing needs explicit control dependency

[Robbin Ehn]

Hi Markus,

This looks okay!

/Robbin

('r'eviewer and not a compiler expert)

On 10/26/2016 04:52 PM, Markus Gronlund wrote:
> Thanks Vladimir!
>
> Can I ask for a second review on this as well?
>
> I would like to fix some test issues relating to this fix as soon as possible.
>
> Thanks in advance
> Markus
>
> -----Original Message-----
> From: Vladimir Kozlov
> Sent: den 26 oktober 2016 16:40
> To: Markus Gronlund
> Cc: hotspot-compiler-dev at openjdk.java.net
> Subject: Re: RFR(XS): 8168662: Intrinsic support for event based tracing needs explicit control dependency
>
> Good.
>
> thanks,
> Vladimir
>
> On 10/26/16 4:18 AM, Markus Gronlund wrote:
>> Thanks a lot for taking a look Vladimir,
>>
>> Good points - I have updated accordingly.
>>
>> (please see updated webrev):
>>
>> http://cr.openjdk.java.net/~mgronlun/8168662/webrev02/
>>
>> Thanks again
>> Markus
>>
>> -----Original Message-----
>> From: Vladimir Kozlov
>> Sent: den 26 oktober 2016 03:05
>> To: Markus Gronlund; hotspot-compiler-dev at openjdk.java.net
>> Subject: Re: RFR(XS): 8168662: Intrinsic support for event based
>> tracing needs explicit control dependency
>>
>> I missed this in review :(
>>
>> You don't need to use set_control() I think. You should just use jobj_is_not_null:
>>
>> Node* res = make_load(jobj_is_not_null, jobj, TypeInstPtr::NOTNULL,
>> T_OBJECT, MemNode::unordered);
>>
>> But I don't insist on this.
>>
>> Also set_result() will do record_for_igvn(region) so you don't need it.
>>
>> Thanks,
>> Vladimir
>>
>> On 10/25/16 4:47 PM, Markus Gronlund wrote:
>>> Greetings,
>>>
>>> I recently integrated intrinsic support for event based tracing which was tracked in JDK-8166806 (https://bugs.openjdk.java.net/browse/JDK-8166806 ).
>>>
>>> Unfortunately, the changes for 8166806 led to issues seen in testing
>>> on SPARC and AARCH64 platforms in that the intrinsic code was missing
>>> an explicit control dependency for C2. On the x86 platform, it seems that there is an implicit control dependency that makes the original code work correctly - but on the former platforms, the lack of dependency allows a load to "float" up before the implicit null check dispatch for the uncommon trap.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8168662
>>>
>>> Webrev: http://cr.openjdk.java.net/~mgronlun/8168662/webrev01/
>>>
>>> I have managed to reproduce and analyze the assembler output for SPARC with the updated changes (please see bug for details).
>>>
>>> I would need to integrate this to resolve some testing issues, so reviews very much appreciated.
>>>
>>> Thanks in advance and sorry for any inconveniences related to 8166806.
>>>
>>> Best regards
>>>
>>> Markus
>>>
>>> PS also thanks for Nils Eliasson for assistance on this issue.
>>>