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

Gustavo Serra Scalet gustavo.scalet at eldorado.org.br
Fri Aug 25 18:50:55 UTC 2017 

Hi Martin,

> -----Original Message-----
> From: Doerr, Martin 
>
> You have mentioned a paper which you have used. I think it should be
> referenced in the comments.

I've used the following paper:
http://www.iwar.org.uk/comsec/resources/cipher/sha256-384-512.pdf

You may add it wherever you feel comfortable.

> 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.

I see. Unfortunately I don't have access to a big endian Linux and it's also not the focus of my current project to improve big endian architectures, so I can't spend too much time on this. But to the best of my efforts, these are the changes that I see that need to be done (please test it then):

$ hg diff
diff -r c0e9986ddfdb src/cpu/ppc/vm/macroAssembler_ppc_sha.cpp
--- a/src/cpu/ppc/vm/macroAssembler_ppc_sha.cpp Fri Aug 25 12:31:12 2017 -0300
+++ b/src/cpu/ppc/vm/macroAssembler_ppc_sha.cpp Fri Aug 25 15:45:58 2017 -0300
@@ -159,6 +159,7 @@

   bind(after_w_load);

+#if defined(VM_LITTLE_ENDIAN)
   // Byte swapping on little endian
   li       (tmp, 8);
   lvsl     (vt0, tmp);
@@ -168,6 +169,7 @@
     VectorRegister w = ws[n];
     vperm  (w,   w,   w,   vt1);
   }
+#endif

   // Loading k, which is always aligned to 16-bytes
   lvx    (kpws[0], k);
@@ -923,6 +925,7 @@
   VectorRegister vsp8 = VR9;
   vspltish(vsp8,     8);

+#if defined(VM_LITTLE_ENDIAN)
   // Convert input from Big Endian to Little Endian
   for (int c = 0; c < total_ws; c++) {
     VectorRegister w = ws[c];
@@ -936,6 +939,7 @@
     VectorRegister w = ws[c];
     vrld  (w, w, vsp32);
   }
+#endif

   Register Rb        = R10;
   VectorRegister vRb = VR8;

> 
> 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

More information about the ppc-aix-port-dev mailing list