RFR: 8073108: GHASH Intrinsics

[Vladimir Kozlov]

On 2/17/15 10:31 AM, Anthony Scarpino wrote:
> On 02/16/2015 06:01 PM, Vladimir Kozlov wrote:
>> On 2/16/15 5:23 PM, David Holmes wrote:
>>> Hi Tony,
>>>
>>> Not a review as hotspot compiler folk need to review this.
>>>
>>> On 17/02/2015 7:11 AM, Anthony Scarpino wrote:
>>>> Hi,
>>>>
>>>> I'm requesting a code review to intrinsify the GHASH operations for
>>>> both
>>>> x86 and SPARC platforms.  This greatly increases performance over
>>>> software for AES/GCM crypto operations, details are in the bug.
>>>
>>> This may be normal for intrinsics but there seems to be a large amount
>>> of shared code being updated to support these platform specific
>>> enhancements. What happens on other platforms if the user sets
>>> UseGHASHIntrinsics? Shouldn't there be a guard against this in
>>> arguments.cpp?
>>
>> Code in vm_version_<arch>.cpp does the check. vm_version_sparc.cpp and
>> vm_version_x86.cpp do that.
>>
>> Tony, please, fix other vm_version_<arch>.cpp files (including closed)
>> to set corresponding flag to false. See UseAES as example.
>
> Ok.. thanks Dave & Vladimir for bring that up.. I fix that..
>
>
>> Also code in vm_version_sparc.cpp should be similar to one in
>> vm_version_x86.cpp. Something like:
>>
>>    // GHASH/GCM intrinsics
>>    if (has_vis3() && (UseVIS > 2)) {
>>      if (FLAG_IS_DEFAULT(UseGHASHIntrinsics)) {
>>        UseGHASHIntrinsics = true;
>>      }
>>    } else if (UseGHASHIntrinsics) {
>>      if (!FLAG_IS_DEFAULT(UseGHASHIntrinsics))
>>        warning("GHASH intrinsics require VIS3 insructions support.
>> Intriniscs will be disabled");
>>      FLAG_SET_DEFAULT(UseGHASHIntrinsics, false);
>>    }
>>
>
> Sure..
>
>> There is #ifdef _WIN64 in stubGenerator_x86_32.cpp. It is not needed
>> since it is 32-bit code.
>
> Sure.. Do I still need to save the xmm6 and xmm7 on 32bit code, or is
> all of the register saving only for Windows 64bit?

No, in 32-bit you don't need to save xmm6 and xmm7.

>
>>
>> I see you switched off -DcheckOutput=false for GCM testing and return
>> from compareArrays() after length compare. Is it because it can't be
>> done or you did not have time to add needed code?
>
> I'll have to double check, but I believe -DcheckOutput can be turned
> back on.  I suspect I never updated the @run lines after I changed
> compareArrays()

In void compareArrays(byte b[], byte exp[]) {

You have:

+    if (mode.equals("GCM"))
+      return;
      for (int i=0; i< exp.length; i++) {
        if (b[i] != exp[i]) {

So current changes do not compare arrays elements for GCM. That is why I 
asked.

Thanks,
Vladimir

>
>> Otherwise Hotspot code looks good.
>>
>> Thanks,
>> Vladimir
>>
>>>
>>> Thanks,
>>> David
>>>
>>>> The review is for two repos, hotspot and jdk:
>>>>
>>>> http://cr.openjdk.java.net/~ascarpino/8073108/hotspot/webrev/
>>>>
>>>> http://cr.openjdk.java.net/~ascarpino/8073108/jdk/webrev/
>>>>
>>>> thanks
>>>>
>>>> Tony
>