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

[Doerr, Martin]

Hi Gustavo,

thanks for your reply.

Ah, x86 uses this naming convention. Looks unusual, but I think we can live with _ppc_sha.cpp.

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.

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