Ping: [8u] request for approval: "8152172: PPC64: Support AES intrinsics"

Tue Aug 9 09:03:31 UTC 2016

Hi Volker,

On 08/08/2016 03:43 PM, Volker Simonis wrote:
> Ping...
>
> Can I please get a review for this downport. The shared changes all
> cleanly apply to jdk8, only the ppc64 part required a small tweak (see
> intial mail).

you changes look good for me. I can sponsor the changes, if you want.

Thank you and best regards,

Zoltan

>
> Thanks,
> Volker
>
> On Wed, Aug 3, 2016 at 1:32 PM, Seán Coffey <sean.coffey at oracle.com> wrote:
>> On 02/08/16 20:47, Volker Simonis wrote:
>>> Hi Sean,
>>>
>>> thanks a lot for your reply.
>>>
>>> You're right - I'm still waiting for a review of the hotspot part. Any
>>> volunteers :)
>> OK - would be good to get a hotspot reviewer to look at those first.
>>>
>>> Regarding the noreg label: I used 'noreg-other' because there already
>>> exist AES tests (they have never been part of this original change).
>>> Is that the right label? There are simply too many different noreg
>>> labels without documentation so if I selected the wrong one, please
>>> advice which one to choose.
>> noreg labels are documented at :
>> http://openjdk.java.net/guide/changePlanning.html#noreg
>> noreg-other seems fine. You might want to add a short comment to bug
>> outlining your comment on existing AES tests.
>>
>> regards,
>> Sean.
>>
>>> Thanks,
>>> Volker
>>>
>>>
>>> On Tue, Aug 2, 2016 at 9:58 AM, Seán Coffey <sean.coffey at oracle.com>
>>> wrote:
>>>> Volker,
>>>>
>>>> Have the jdk8u hotspot edits been reviewed ? Looks like they should be.
>>>>
>>>> Can you add a suitable noreg label to the master bug report also.
>>>>
>>>> Regards,
>>>> Sean.
>>>>
>>>>
>>>> On 29/07/2016 19:58, Volker Simonis wrote:
>>>>> Ping!
>>>>>
>>>>> Can I please have an approval for backporting this change to 8u-dev?
>>>>>
>>>>> Thanks,
>>>>> Volker
>>>>>
>>>>>
>>>>> On Fri, Jul 22, 2016 at 11:08 AM, Volker Simonis
>>>>> <volker.simonis at gmail.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> could you please approve the backport of the following, mostly ppc64
>>>>>> change to jdk8u-dev:
>>>>>>
>>>>>> 8152172: PPC64: Support AES intrinsics
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8152172
>>>>>> Webrev: http://cr.openjdk.java.net/~simonis/webrevs/2016/8152172_8u_hs/
>>>>>> Webrev http://cr.openjdk.java.net/~simonis/webrevs/2016/8152172_8u_jdk/
>>>>>> Review:
>>>>>>
>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2016-March/thread.html#22032
>>>>>> URL:http://hg.openjdk.java.net/jdk9/hs-comp/jdk/rev/74bc7be0777b
>>>>>> URL:http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/rev/68394bf0a09f
>>>>>>
>>>>>> As you can see, the change consists of two parts - the main one in the
>>>>>> hotpsot repo and a small part in the jdk repo.
>>>>>>
>>>>>> The jdk part applied cleanly to jdk8u (with the usual directory
>>>>>> shuffling).
>>>>>>
>>>>>> The hotspot part required a small tweak, but only in the ppc-only
>>>>>> part. This is because the feature detection for the AES instructions
>>>>>> was already active in jdk9 when the original change was made but is
>>>>>> not available in jdk8u until now. You can find the additional changes
>>>>>> at the end of this mail for your convenience.
>>>>>>
>>>>>> The required shared changes cleanly apply to jdk8u.
>>>>>>
>>>>>> As Vladimir pointed out in a previous thread, shared Hotspot changes
>>>>>> have to go through JPRT even in jdk8u-dev. So I need a sponsor to do
>>>>>> that and synchronously push both parts.
>>>>>>
>>>>>> @Hiroshii: could you please verify that you are fine with the change?
>>>>>> I've done some tests on Power8LE but just want to be sure this
>>>>>> downport satisfies your needs as well.
>>>>>>
>>>>>> Thank you and best regards,
>>>>>> Volker
>>>>>>
>>>>>> =====================
>>>>>>
>>>>>> diff -r 15928d255046 src/cpu/ppc/vm/vm_version_ppc.cpp
>>>>>> --- a/src/cpu/ppc/vm/vm_version_ppc.cpp Wed Jul 13 00:47:40 2016 -0700
>>>>>> +++ b/src/cpu/ppc/vm/vm_version_ppc.cpp Fri Jul 22 10:32:36 2016 +0200
>>>>>> @@ -452,6 +476,7 @@
>>>>>>       a->popcntw(R7, R5);                          // code[7] -> popcntw
>>>>>>       a->fcfids(F3, F4);                           // code[8] -> fcfids
>>>>>>       a->vand(VR0, VR0, VR0);                      // code[9] -> vand
>>>>>> +  a->vcipher(VR0, VR1, VR2);                   // code[10] -> vcipher
>>>>>>       a->blr();
>>>>>>
>>>>>>       // Emit function to set one cache line to zero. Emit function
>>>>>> descriptor and get pointer to it.
>>>>>> @@ -495,6 +520,7 @@
>>>>>>       if (code[feature_cntr++]) features |= popcntw_m;
>>>>>>       if (code[feature_cntr++]) features |= fcfids_m;
>>>>>>       if (code[feature_cntr++]) features |= vand_m;
>>>>>> +  if (code[feature_cntr++]) features |= vcipher_m;
>>>>>>
>>>>>>       // Print the detection code.
>>>>>>       if (PrintAssembly) {
>>>>>> diff -r 15928d255046 src/cpu/ppc/vm/vm_version_ppc.hpp
>>>>>> --- a/src/cpu/ppc/vm/vm_version_ppc.hpp Wed Jul 13 00:47:40 2016 -0700
>>>>>> +++ b/src/cpu/ppc/vm/vm_version_ppc.hpp Fri Jul 22 10:32:36 2016 +0200
>>>>>> @@ -42,6 +42,7 @@
>>>>>>         fcfids,
>>>>>>         vand,
>>>>>>         dcba,
>>>>>> +    vcipher,
>>>>>>         num_features // last entry to count features
>>>>>>       };
>>>>>>       enum Feature_Flag_Set {
>>>>>> @@ -56,6 +57,7 @@
>>>>>>         fcfids_m              = (1 << fcfids ),
>>>>>>         vand_m                = (1 << vand   ),
>>>>>>         dcba_m                = (1 << dcba   ),
>>>>>> +    vcipher_m             = (1 << vcipher),
>>>>>>         all_features_m        = -1
>>>>>>       };
>>>>>>       static int  _features;
>>>>>> @@ -83,6 +85,7 @@
>>>>>>       static bool has_fcfids()  { return (_features & fcfids_m) != 0; }
>>>>>>       static bool has_vand()    { return (_features & vand_m) != 0; }
>>>>>>       static bool has_dcba()    { return (_features & dcba_m) != 0; }
>>>>>> +  static bool has_vcipher() { return (_features & vcipher_m) != 0; }
>>>>>>
>>>>>>       static const char* cpu_features() { return _features_str; }
>>>>