[10] RFR(L): 8185979: PPC64: Implement SHA2 intrinsic

[Doerr, Martin]

Hi Gustavo,

thanks for the new webrev.

You have mentioned a paper which you have used. I think it should be referenced in the comments.

I think you didn't get my point about AIX.
Your current version doesn't break AIX, but it lacks SHA2 acceleration for AIX on Power 8 and newer, which is still relevant.
So I'd like to ask you kindly to take a look if Big Endian support for the stub could be added without high effort. AIX doesn't need VRSAVE handling (like Little Endian linux, unlike Big Endian linux), so a few lines in the stub could possibly be enough. I can assist with testing.

Thanks for doing the other changes which I had proposed.

Best regards,
Martin


-----Original Message-----
From: Gustavo Serra Scalet [mailto:gustavo.scalet at eldorado.org.br] 
Sent: Freitag, 25. August 2017 17:54
To: Doerr, Martin <martin.doerr at sap.com>; 'hotspot-compiler-dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>
Cc: ppc-aix-port-dev at openjdk.java.net; Vladimir Kozlov <vladimir.kozlov at oracle.com>
Subject: RE: [10] RFR(L): 8185979: PPC64: Implement SHA2 intrinsic

Hi Martin and Vladimir,

I'm cc'ing Vladimir in hope to get some review from Oracle.

The following is the updated webrev:
https://gut.github.io/openjdk/webrev/JDK-8185979/webrev.01/

Also I remembered the reason why I was using "this->b" when recompiling: there is a common variable named "b" (as I used the naming convention of a SHA-2 paper calling each of the 8 state elements "a" through "h") so that name was taken on those scopes. 

A cool nit is that I actually found parts that I didn't need the "b" variable despite being declared so I cleaned the sha256_load_h_vec interface and, on other spots, I renamed the "b" variable to "b_". Now I could use "b" instruction directly.

> -----Original Message-----
> From: Doerr, Martin 
>
> Ah, x86 uses this naming convention. Looks unusual, but I think we can
> live with _ppc_sha.cpp.

Alright, I'm keeping that then.

> Don't forget that openJDK supports AIX which is a Big Endian PPC64
> platform. I think we can disable the code for Big Endian linux as it
> would require additional VRSAVE handling and I agree with that this
> platform is not relevant for Power 8. However, AIX is still relevant for
> modern Power processors.

I see. But then is my change on "vm_version_ppc.cpp" enough? (Around the "#if defined(VM_LITTLE_ENDIAN)", on https://gut.github.io/openjdk/webrev/JDK-8185979/webrev.01/src/cpu/ppc/vm/vm_version_ppc.cpp.udiff.html )


Thanks once again

> We are only allowed to sponsor changes in jdk or hotspot PPC64/s390
> files. As soon as you touch any other hotspot file, you will need a
> sponsor from Oracle. I suggest that you ask for 2 SAP reviews, maybe
> somebody from IBM can also review as additional reviewer. It should be
> possible to find a sponsor from Oracle after the PPC files are
> thoroughly reviewed.
>
> Thanks again and best regards,
> Martin
> 
> 
> -----Original Message-----
> From: Gustavo Serra Scalet [mailto:gustavo.scalet at eldorado.org.br]
> Sent: Freitag, 25. August 2017 14:57
> To: Doerr, Martin <martin.doerr at sap.com>; 'hotspot-compiler-
> dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>
> Cc: ppc-aix-port-dev at openjdk.java.net
> Subject: RE: [10] RFR(L): 8185979: PPC64: Implement SHA2 intrinsic
> 
> Hi Martin,
> 
> Thanks for the review!
> 
> > -----Original Message-----
> > From: Doerr, Martin
> > - The file name of the new file should end with _ppc.cpp.
> 
> It contains already the _ppc, but not as a suffix:
> "macroAssembler_ppc_sha.cpp"
> I followed the same idea behind other existing files like
> "macroAssembler_x86_log10.cpp", "macroAssembler_x86_pow.cpp",
> "macroAssembler_x86_tan.cpp"...
> 
> Do you want me to change it to "macroAssembler_sha_ppc.cpp" ?
> 
> > - "this->" should be removed from branch instructions: this->b
> 
> Right! Thanks for spotting it.
> 
> > - Why are you using Register references "const Register&"? That's not
> > common.
> 
> For a moment I thought that "Register" was a structure so a reference
> would reduce initialization of that type, but analyzing now, it's a
> pointer. Therefore there is no reason to pass a reference to a pointer
> on my function.
> 
> I'm changing that. Thanks
> 
> > - It would be nice to have Big Endian support if it doesn't take much
> > effort. (I can assist with testing.)
> 
> I don't see the reason for that because the vshasigmaw/vshasigmad are
> only available on POWER8 and beyond. Those systems are, AFAIK, little
> endian only, right?
> 
> Otherwise I will need to reconsider some steps. But please advise if
> that will be useful for some platform.
> 
> > Note: This change contains test changes, so a sponsor from Oracle is
> > needed.
> 
> I didn't add tests. I only enabled them to ppc now which should be easy
> to review.
> 
> How do I ask for that kind of sponsor?
> 
> Thanks
> 
> >
> > Best regards,
> > Martin
> >
> >
> > -----Original Message-----
> > From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-
> > bounces at openjdk.java.net] On Behalf Of Gustavo Serra Scalet
> > Sent: Donnerstag, 17. August 2017 21:06
> > To: 'hotspot-compiler-dev at openjdk.java.net' <hotspot-compiler-
> > dev at openjdk.java.net>
> > Cc: ppc-aix-port-dev at openjdk.java.net
> > Subject: [10] RFR(L): 8185979: PPC64: Implement SHA2 intrinsic
> >
> > Hi,
> >
> > Could you please review this specific PPC64 change to hotspot? By
> > implementing this intrinsic I noticed a significant improvement when
> > using SHA-2 (e.g: Sample run reduced 6.5s run to 2.8s for SHA256.)
> >
> > JBS: https://bugs.openjdk.java.net/browse/JDK-8185979
> > Webrev: https://gut.github.io/openjdk/webrev/JDK-8185979/webrev/
> >
> > Best regards,
> > Gustavo Serra Scalet