PING Re: RFR: 8163589: Add back class id intrinsic method for event based tracing

[Robbin Ehn]

Hi,

On 08/29/2016 07:22 PM, Vladimir Kozlov wrote:
> Sorry, my bad - C2 supports long shifts in 32-bit JVM unconditionally. Your previous hotspot.04/webrev/ looks fine then regarding C2.

Ok, thanks.

>
> But :) C1 restriction could be improved. I looked on jdk 8 code (wondering it works) and it does not have shift.

Yes, shift is new in jdk9.

>
> Assuming that shift is not always defined the code in c1_Compiler.cpp could be:
>
> +#if defined(_LP64) || !defined(TRACE_ID_CLASS_SHIFT)
> +  case vmIntrinsics::_getClassId:
> +#endif

Yes, incremental on top of version 04:
Inc: http://cr.openjdk.java.net/~rehn/8163589/hotspot.04-06/
Full: http://cr.openjdk.java.net/~rehn/8163589/hotspot.06/

Tested with jmh x64/x86 with PrintIntrinsics/PrintInlining.

Markus you looked at v04, I hope you are also are fine with this?

Thanks!

/Robbin

>
> Thanks,
> Vladimir
>
> On 8/29/16 5:10 AM, Robbin Ehn wrote:
>> Thanks for looking at this again,
>>
>> On 08/26/2016 08:28 PM, Vladimir Kozlov wrote:
>>> C2 code also should be guarded by #ifdef _LP64
>>
>> Here is the webrev for that:
>> Inc:http://cr.openjdk.java.net/~rehn/8163589/hotspot.04-05/webrev/
>> Full:http://cr.openjdk.java.net/~rehn/8163589/hotspot.05/webrev/
>>
>> Tested with jmh, jdk_jfr and new intrinsic test(closed).
>>
>> It's not obvious to me why, since tests work on at least x86.
>> Would you mind sharing why this is needed?
>>
>> Thanks!
>>
>> /Robbin
>>
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 8/26/16 12:36 AM, Robbin Ehn wrote:
>>>> Hi all,
>>>>
>>>> Please have a look, thanks!
>>>>
>>>> /Robbin
>>>>
>>>> (updated issue heading also)
>>>>
>>>> On 08/23/2016 06:06 PM, Robbin Ehn wrote:
>>>>> Hi all,
>>>>>
>>>>> C1 in 32-bit actually don't support shift operation which was added in
>>>>> v3 of this series.
>>>>>
>>>>> We hit this assert in 32-bit:
>>>>> #  Internal Error
>>>>> (/home/rehn/source/jdk/8163589/hotspot/src/cpu/x86/vm/c1_LIRAssembler_x86.cpp:2917),
>>>>>
>>>>>
>>>>> pid=22445, tid=22493
>>>>> #  Error: Unimplemented()
>>>>>
>>>>> So we will just live without this intrinsic in C1 32-bit.
>>>>>
>>>>> Also added specific test for intrinsic and 'event based testing', see
>>>>> bug (https://bugs.openjdk.java.net/browse/JDK-8163589).
>>>>>
>>>>> I really hope I haven't missed anything else!
>>>>>
>>>>> Incr:
>>>>> http://cr.openjdk.java.net/~rehn/8163589/hotspot.03-04/webrev/
>>>>>
>>>>> Full:
>>>>> http://cr.openjdk.java.net/~rehn/8163589/hotspot.04/webrev/
>>>>>
>>>>> Thanks!
>>>>>
>>>>> /Robbin
>>>>>
>>>>> On 08/10/2016 04:05 PM, Robbin Ehn wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> In 8 we had this intrinsic, I'm adding it back
>>>>>> Used for event based tracing.
>>>>>>
>>>>>> Webrev: http://cr.openjdk.java.net/~rehn/8163589/hotspot.01/webrev/
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8163589
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> /Robbin