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

[Markus Gronlund]

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