RFR (M): 8150767: Update for x86 SHA Extensions enabling

Christian Thalinger christian.thalinger at oracle.com
Thu Mar 3 22:54:47 UTC 2016 

Much better.  Thanks.

> On Mar 3, 2016, at 12:43 PM, Deshpande, Vivek R <vivek.r.deshpande at intel.com> wrote:
> 
> Hi Vladimir, Christian
> 
> I have updated the code with #ifdef.
> Please find the updated webrev here:
> http://cr.openjdk.java.net/~vdeshpande/SHANI/8150767/webrev.03/
> 
> Regards,
> Vivek
> 
> 
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com] 
> Sent: Wednesday, March 02, 2016 2:14 PM
> To: Christian Thalinger; Deshpande, Vivek R
> Cc: hotspot compiler; Rukmannagari, Shravya
> Subject: Re: RFR (M): 8150767: Update for x86 SHA Extensions enabling
> 
> On 3/2/16 2:04 PM, Christian Thalinger wrote:
>> 
>>> On Mar 2, 2016, at 11:49 AM, Deshpande, Vivek R 
>>> <vivek.r.deshpande at intel.com <mailto:vivek.r.deshpande at intel.com>> wrote:
>>> 
>>> Hi Christian
>>> 
>>> We could combine the declaration of the functions such as
>>> fast_sha256() for 32 bit and 64 bit in macroAssembler_x86.hpp using COMMA.
>>> We used COMMA to separate more arguments used in 64 bit using 
>>> LP64_ONLY macro.
>> 
>> Ugh.  This is ugly:
>> 
>> voidfast_pow(XMMRegisterxmm0, XMMRegisterxmm1, XMMRegisterxmm2, 
>> XMMRegisterxmm3, XMMRegisterxmm4, XMMRegisterxmm5, XMMRegisterxmm6, 
>> XMMRegisterxmm7, Registerrax, Registerrcx, Register rdx NOT_LP64(COMMA  
>> Register tmp) LP64_ONLY(COMMA Register tmp1)
>>                 LP64_ONLY(COMMA Register tmp2) LP64_ONLY(COMMA 
>> Register
>> tmp3) LP64_ONLY(COMMA Register tmp4));
>> 
>> Can’t we use #ifdef instead?
> 
> +1 for #ifdef
> It is not first time we scope additional parameters with #ifdef.
> 
> Vladimir
> 
>> 
>>> 
>>> Regards,
>>> Vivek
>>> 
>>> -----Original Message-----
>>> From: Christian Thalinger [mailto:christian.thalinger at oracle.com]
>>> Sent: Wednesday, March 02, 2016 9:30 AM
>>> To: Deshpande, Vivek R
>>> Cc: Vladimir Kozlov; hotspot compiler; Rukmannagari, Shravya
>>> Subject: Re: RFR (M): 8150767: Update for x86 SHA Extensions enabling
>>> 
>>> #define COMMA ,
>>> 
>>> Why do we have a define like this?  It came in with 8139575.
>>> 
>>>> On Mar 1, 2016, at 3:24 PM, Deshpande, Vivek R 
>>>> <vivek.r.deshpande at intel.com <mailto:vivek.r.deshpande at intel.com>> wrote:
>>>> 
>>>> Hi Vladimir, Christian
>>>> 
>>>> I have updated the code according your suggestion of file name change.
>>>> The updated webrev is at this location:
>>>> http://cr.openjdk.java.net/~vdeshpande/SHANI/8150767/webrev.02/
>>>> Please let me know if I have to do anything more.
>>>> Also is there any change required to Makefile so that 
>>>> configurations.xml has name of the added file ?
>>>> 
>>>> Regards,
>>>> Vivek
>>>> 
>>>> -----Original Message-----
>>>> From: Christian Thalinger [mailto:christian.thalinger at oracle.com]
>>>> Sent: Monday, February 29, 2016 5:59 PM
>>>> To: Vladimir Kozlov
>>>> Cc: Deshpande, Vivek R; hotspot compiler; Rukmannagari, Shravya
>>>> Subject: Re: RFR (M): 8150767: Update for x86 SHA Extensions 
>>>> enabling
>>>> 
>>>> 
>>>>> On Feb 29, 2016, at 3:15 PM, Vladimir Kozlov 
>>>>> <vladimir.kozlov at oracle.com> wrote:
>>>>> 
>>>>> I am against to have "intel" in a file name. We have 
>>>>> macroAssembler_libm_x86_*.cpp files for math intrinsics which does 
>>>>> not have intel in name. So I prefer to not have it. I would suggest 
>>>>> macroAssembler_sha_x86.cpp.
>>>> 
>>>> I know we already have macroAssembler_libm_x86_*.cpp but 
>>>> macroAssembler_x86_<feature>.cpp would be better.
>>>> 
>>>>> You can manipulate when to use it in vm_version_x86.cpp.
>>>>> 
>>>>> Intel Copyright in the file's header is fine.
>>>>> 
>>>>> Code changes are fine now (webrev.01).
>>>>> 
>>>>> Thanks,
>>>>> Vladimir
>>>>> 
>>>>> On 2/29/16 4:42 PM, Christian Thalinger wrote:
>>>>>> 
>>>>>>> On Feb 29, 2016, at 2:00 PM, Deshpande, Vivek R 
>>>>>>> <vivek.r.deshpande at intel.com> wrote:
>>>>>>> 
>>>>>>> Hi Christian
>>>>>>> 
>>>>>>> We used the SHA Extension
>>>>>>> implementations(https://software.intel.com/en-us/articles/intel-s
>>>>>>> ha-extensions-implementations) for the JVM implementation of SHA1 
>>>>>>> and SHA256.
>>>>>> 
>>>>>> Will that extension only be available on Intel chips?
>>>>>> 
>>>>>>> It needed to have Intel copyright, so created a separate file.
>>>>>> 
>>>>>> That is reasonable.
>>>>>> 
>>>>>>> The white paper for the implementation is 
>>>>>>> https://software.intel.com/sites/default/files/article/402097/intel-sha-extensions-white-paper.pdf.
>>>>>>> 
>>>>>>> Regards,
>>>>>>> Vivek
>>>>>>> 
>>>>>>> -----Original Message-----
>>>>>>> From: Christian Thalinger [mailto:christian.thalinger at oracle.com]
>>>>>>> Sent: Monday, February 29, 2016 1:58 PM
>>>>>>> To: Deshpande, Vivek R
>>>>>>> Cc: Vladimir Kozlov; hotspot compiler; Rukmannagari, Shravya
>>>>>>> Subject: Re: RFR (M): 8150767: Update for x86 SHA Extensions 
>>>>>>> enabling
>>>>>>> 
>>>>>>> Why is the new file called macroAssembler_intel_x86.cpp?
>>>>>>> 
>>>>>>>> On Feb 29, 2016, at 11:29 AM, Deshpande, Vivek R 
>>>>>>>> <vivek.r.deshpande at intel.com> wrote:
>>>>>>>> 
>>>>>>>> HI Vladimir
>>>>>>>> 
>>>>>>>> Thank you for your review.
>>>>>>>> I have updated the patch with the changes you have suggested.
>>>>>>>> The new webrev is at this location:
>>>>>>>> http://cr.openjdk.java.net/~vdeshpande/SHANI/8150767/webrev.01/
>>>>>>>> 
>>>>>>>> Regards
>>>>>>>> Vivek
>>>>>>>> 
>>>>>>>> -----Original Message-----
>>>>>>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>>>>>>> Sent: Friday, February 26, 2016 6:50 PM
>>>>>>>> To: Deshpande, Vivek R; hotspot compiler
>>>>>>>> Cc: Viswanathan, Sandhya; Rukmannagari, Shravya
>>>>>>>> Subject: Re: RFR (M): 8150767: Update for x86 SHA Extensions 
>>>>>>>> enabling
>>>>>>>> 
>>>>>>>> Very nice, Vivek!!!
>>>>>>>> 
>>>>>>>> Did you run tests with both 32- and 64-bit VMs?
>>>>>>>> 
>>>>>>>> Small notes:
>>>>>>>> 
>>>>>>>> In vm_version_x86.hpp spacing are not aligned in next line:
>>>>>>>> 
>>>>>>>> static bool supports_avxonly()    { return ((supports_avx2() ||
>>>>>>>> supports_avx()) && !supports_evex()); }
>>>>>>>> +  static bool supports_sha()      { return (_features & CPU_SHA)
>>>>>>>> != 0; }
>>>>>>>> 
>>>>>>>> Flags setting code in vm_version_x86.cpp should be like this 
>>>>>>>> (you can check supports_sha() only once, don't split '} else {' 
>>>>>>>> line, set UseSHA false if all intrinsics flags are false (I 
>>>>>>>> included UseSHA512Intrinsics for future) ):
>>>>>>>> 
>>>>>>>> if (supports_sha()) {
>>>>>>>> if (FLAG_IS_DEFAULT(UseSHA)) {
>>>>>>>>   UseSHA = true;
>>>>>>>> }
>>>>>>>> } else if (UseSHA) {
>>>>>>>> warning("SHA instructions are not available on this CPU");  
>>>>>>>> FLAG_SET_DEFAULT(UseSHA, false);  }
>>>>>>>> 
>>>>>>>> if (UseSHA) {
>>>>>>>> if (FLAG_IS_DEFAULT(UseSHA1Intrinsics)) {
>>>>>>>>   FLAG_SET_DEFAULT(UseSHA1Intrinsics, true);  } } else if 
>>>>>>>> (UseSHA1Intrinsics) {  warning("Intrinsics for SHA-1 crypto hash 
>>>>>>>> functions not available on this CPU.");  
>>>>>>>> FLAG_SET_DEFAULT(UseSHA1Intrinsics, false);  }
>>>>>>>> 
>>>>>>>> if (UseSHA) {
>>>>>>>> if (FLAG_IS_DEFAULT(UseSHA256Intrinsics)) {
>>>>>>>>   FLAG_SET_DEFAULT(UseSHA256Intrinsics, true);  } } else if 
>>>>>>>> (UseSHA256Intrinsics) {  warning("Intrinsics for SHA-224 and 
>>>>>>>> SHA-256 crypto hash functions not available on this CPU.");  
>>>>>>>> FLAG_SET_DEFAULT(UseSHA256Intrinsics, false);  }
>>>>>>>> 
>>>>>>>> if (UseSHA512Intrinsics) {
>>>>>>>> warning("Intrinsics for SHA-384 and SHA-512 crypto hash 
>>>>>>>> functions not available on this CPU.");  
>>>>>>>> FLAG_SET_DEFAULT(UseSHA512Intrinsics, false);  }
>>>>>>>> 
>>>>>>>> if (!(UseSHA1Intrinsics || UseSHA256Intrinsics ||
>>>>>>>> UseSHA512Intrinsics)) {
>>>>>>>> FLAG_SET_DEFAULT(UseSHA, false);  }
>>>>>>>> 
>>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> Vladimir
>>>>>>>> 
>>>>>>>> On 2/26/16 4:37 PM, Deshpande, Vivek R wrote:
>>>>>>>>> Hi all
>>>>>>>>> 
>>>>>>>>> I would like to contribute a patch which optimizesSHA-1
>>>>>>>>> andSHA-256 for
>>>>>>>>> 64 and 32 bitX86architecture using Intel SHA extensions.
>>>>>>>>> 
>>>>>>>>> Could you please review and sponsor this patch.
>>>>>>>>> 
>>>>>>>>> Bug-id:
>>>>>>>>> 
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8150767
>>>>>>>>> webrev:
>>>>>>>>> 
>>>>>>>>> http://cr.openjdk.java.net/~vdeshpande/SHANI/8150767/webrev.00/
>>>>>>>>> 
>>>>>>>>> Thanks and regards,
>>>>>>>>> 
>>>>>>>>> Vivek
>>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>> 
>>> 
>>

More information about the hotspot-compiler-dev mailing list