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

Tue Sep 12 16:32:27 UTC 2017

Hi Gustavo,

thanks for debugging. It should fix the problem for LE, but the previous version was correct for BE.

Version which works for both:
http://cr.openjdk.java.net/~mdoerr/8185979_ppc_sha2/webrev.04/

Changes:
- factored out lvsr/lvsl
- fixed ofs <= limit comparison (treat as positive ints to ignore garbage in high half and be protected against integer overflow, use <= see DigestBase.java)
- removed unused labels
- added the contributors you mentioned to Contributed-by list (not comments, I think it's better there)

Please let us know if this is ok for you. We'll do some more testing on all platforms.

Best regards,
Martin

-----Original Message-----
From: Gustavo Serra Scalet [mailto:gustavo.scalet at eldorado.org.br] 
Sent: Dienstag, 12. September 2017 17:48
To: Doerr, Martin <martin.doerr at sap.com>; Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Gustavo Romero <gromero at linux.vnet.ibm.com>
Cc: 'hotspot-compiler-dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>; ppc-aix-port-dev at openjdk.java.net
Subject: RE: [10] RFR(L): 8185979: PPC64: Implement SHA2 intrinsic

Hi Martin and Götz,

I was taking a closer look at the hotspot's tests/compiler and I see indeed one test failing for sha:
Passed: compiler/intrinsics/sha/cli/TestUseSHA256IntrinsicsOptionOnSupportedCPU.java
Passed: compiler/intrinsics/sha/cli/TestUseSHA512IntrinsicsOptionOnSupportedCPU.java
Passed: compiler/intrinsics/sha/cli/TestUseSHA1IntrinsicsOptionOnUnsupportedCPU.java
Passed: compiler/intrinsics/sha/cli/TestUseSHAOptionOnUnsupportedCPU.java
Passed: compiler/intrinsics/sha/cli/TestUseSHA512IntrinsicsOptionOnUnsupportedCPU.java
Passed: compiler/intrinsics/sha/cli/TestUseSHA1IntrinsicsOptionOnSupportedCPU.java
Passed: compiler/intrinsics/sha/cli/TestUseSHA256IntrinsicsOptionOnUnsupportedCPU.java
Passed: compiler/intrinsics/sha/cli/TestUseSHAOptionOnSupportedCPU.java
FAILED: compiler/intrinsics/sha/TestSHA.java
Passed: compiler/intrinsics/sha/sanity/TestSHA1Intrinsics.java
Passed: compiler/intrinsics/sha/sanity/TestSHA256Intrinsics.java
Passed: compiler/intrinsics/sha/sanity/TestSHA1MultiBlockIntrinsics.java
Passed: compiler/intrinsics/sha/sanity/TestSHA512Intrinsics.java
Passed: compiler/intrinsics/sha/sanity/TestSHA512MultiBlockIntrinsics.java
Passed: compiler/intrinsics/sha/sanity/TestSHA256MultiBlockIntrinsics.java

That one was failing due to a bug on unaligned memory load. I took a closer look and fixed it. It should also work on Big Endian:
https://gut.github.io/openjdk/webrev/JDK-8185979/webrev.02/index.html

This new webrev was updated on top of Martin's webrev.03.

I also took this chance to add all the contributors to this patch, as you suggested before.

Thanks

> -----Original Message-----
> From: Doerr, Martin [mailto:martin.doerr at sap.com]
> Sent: segunda-feira, 11 de setembro de 2017 14:06
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Gustavo Romero
> <gromero at linux.vnet.ibm.com>; Gustavo Serra Scalet
> <gustavo.scalet at eldorado.org.br>
> Cc: 'hotspot-compiler-dev at openjdk.java.net' <hotspot-compiler-
> dev at openjdk.java.net>; ppc-aix-port-dev at openjdk.java.net
> Subject: RE: [10] RFR(L): 8185979: PPC64: Implement SHA2 intrinsic
> 
> Hi Götz and Gustavo,
> 
> I had just posted the version I had before leaving. Thanks for your
> feedback.
> 
> New webrev is here:
> http://cr.openjdk.java.net/~mdoerr/8185979_ppc_sha2/webrev.03/
> 
> Changes to webrev.02:
> - Referenced paper
> - Factored out endianness specific vector permute instructions (vec_perm
> with only 3 parms to reduce risk of mixing them up)
> - Removed code for PPC64 platforms which didn't support it
> - code_size2 = 22000
> - Added missing ')' in IntrinsicPredicates.java
> 
> My changes shouldn't change the behavior of the little endian
> implementation.
> We have to check if and if yes which tests still fail. Are there any
> updates on this?
> 
> Best regards,
> Martin
> 
> 
> -----Original Message-----
> From: Lindenmaier, Goetz
> Sent: Donnerstag, 7. September 2017 09:55
> To: Gustavo Romero <gromero at linux.vnet.ibm.com>; Doerr, Martin
> <martin.doerr at sap.com>; Gustavo Serra Scalet
> <gustavo.scalet at eldorado.org.br>
> Cc: 'hotspot-compiler-dev at openjdk.java.net' <hotspot-compiler-
> dev at openjdk.java.net>; ppc-aix-port-dev at openjdk.java.net
> Subject: RE: [10] RFR(L): 8185979: PPC64: Implement SHA2 intrinsic
> 
> Hi,
> 
> I had a look at this change.
> 
> Martin, you missed a ')' in IntrinsicPredicates.java.
> 
> Combined with the multiplyToLen change, stub codebuffer space runs out.
> Please increase
>     code_size2 = 20000
> to 22000 in stubRoutines_ppc.hpp.
> 
> I see TestSHA.java failing on linuxppc64le.
> Also, other tests are failing with SHA-256 digest error ...
> 
> Also, on aix, some of our internal tests are failing. These didn't run
> on linuxppc64 on a Power8 machine, so it might fail there, too.  But on
> the big endian platforms, the jtreg tests don't fail.
> 
> @Gustavo, maybe you can have a look at the issues on linuxppc64le and
> post a new webrev. Then Martin can fix the remaining issue on big
> endian.
> 
> Best regards,
>   Goetz.
> 
> 
> 
> 
> > -----Original Message-----
> > From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-
> > bounces at openjdk.java.net] On Behalf Of Gustavo Romero
> > Sent: Freitag, 1. September 2017 18:04
> > To: Doerr, Martin <martin.doerr at sap.com>; Gustavo Serra Scalet
> > <gustavo.scalet at eldorado.org.br>
> > Cc: 'hotspot-compiler-dev at openjdk.java.net' <hotspot-compiler-
> > dev at openjdk.java.net>; ppc-aix-port-dev at openjdk.java.net
> > Subject: Re: [10] RFR(L): 8185979: PPC64: Implement SHA2 intrinsic
> >
> > Hi Martin!
> >
> > On 01-09-2017 12:39, Doerr, Martin wrote:
> > > Hi Gustavos,
> > >
> > > I have managed to upload a version which seems to work on both
> > endianness implementations.
> > > At least some quick tests have passed on AIX and Big Endian linux in
> > addition to Little Endian linux.
> >
> > Great! :-)
> >
> >
> > > I'll be out next week, but the change looks ok for me. Please let me
> > > know if
> > the changed version still looks ok for you, too. Feel free to overwork
> > or improve it.
> > > It'd also be good to know, if relying on vrsave=-1 is safe.
> >
> > Sure, Martin. I'm chasing what's exactly setting vrsave=-1 and the
> > full history log (looks like it's not in the kernel, but I'm checking
> yet).
> >
> >
> > > Is the copyright information ok? Did you get source code which
> > > requires to
> > be mentioned in the comments?
> > > The code looks similar to a reference implementation, so the authors
> > > of it
> > may want to be mentioned?
> > > Or did you just use the paper for implementing it? In this case, I'd
> > > mention
> > the paper.
> >
> > Gustavo S: the information on the paper must be updated accordingly as
> > Martin noted in the new webrev. There is none currently.
> >
> >
> > > After we got a second review and ran more tests, we can ask somebody
> > from Oracle to push it.
> > >
> > > Thanks for contributing and your support, Martin
> >
> > Thanks a lot for reviewing and for all the help.
> >
> > Regards,
> > Gustavo R
> >
> > >
> > > -----Original Message-----
> > > From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-
> > bounces at openjdk.java.net] On Behalf Of Doerr, Martin
> > > Sent: Donnerstag, 31. August 2017 18:21
> > > To: Gustavo Romero <gromero at linux.vnet.ibm.com>
> > > Cc: 'hotspot-compiler-dev at openjdk.java.net' <hotspot-compiler-
> > dev at openjdk.java.net>; ppc-aix-port-dev at openjdk.java.net
> > > Subject: RE: [10] RFR(L): 8185979: PPC64: Implement SHA2 intrinsic
> > >
> > > Hi Gustavo R,
> > >
> > > I guess you're right. vrsave is already set to -1, so all Vector
> > > Registers get
> > saved.
> > > It'd be good to know where it is set (OS, Flag in ELF header, ???)
> > > and if this
> > is guaranteed.
> > > I don't want to risk getting sporadic errors on some OS versions.
> > >
> > > I'd like to enable SHA intrinsics on linux BE as well. I already
> > > managed to get
> > the 256 bit version working (was quite some work!).
> > >
> > > Thanks and best regards,
> > > Martin
> > >
> > >
> > > -----Original Message-----
> > > From: Gustavo Romero [mailto:gromero at linux.vnet.ibm.com]
> > > Sent: Freitag, 25. August 2017 22:35
> > > To: Doerr, Martin <martin.doerr at sap.com>
> > > Cc: Gustavo Serra Scalet <gustavo.scalet at eldorado.org.br>; 'hotspot-
> > compiler-dev at openjdk.java.net' <hotspot-compiler-
> > dev at openjdk.java.net>; ppc-aix-port-dev at openjdk.java.net
> > > Subject: Re: [10] RFR(L): 8185979: PPC64: Implement SHA2 intrinsic
> > >
> > > Hi Martin,
> > >
> > > On 25-08-2017 13:18, Doerr, Martin wrote:
> > >> 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 don't think that VRSAVE is handled on Linux, even on BE. Although
> > > BE ABI
> > [1]
> > > says:
> > >
> > > "Functions must ensure that the appropriate bits in the vrsave
> > > register are
> > set for any vector registers they use"
> > >
> > > and LE ABI does not say that, even on Linux BE VRSAVE is not in
> > > effect used to determine which vector registers (VMX/Altivec) should
> > > be
> > saved/restored.
> > > No application uses it on Linux, so I would say that VRSAVE is
> > > ignored on
> > Linux
> > > completely both on BE and LE. save/restore library interfaces don't
> > > pay attention to it in glibc: VRSAVE is just saved/restored
> > > completely in
> > mechanisms
> > > of swap/get/setcontext(), set/longjump(), and dl-trampoline() and
> > > that's
> > all. I
> > > checked that with toolchain folks and they agree. We've already
> > > discussed
> > that a
> > > long time ago but at that time I was just using the vector-scalar
> > > registers [2] and at that time I agreed that if VMX/Altivec was in
> > > use instead of the VSX
> > so
> > > VRSAVE should be handled accordingly. But I have a different opinion
> > now...
> > >
> > > I'm wondering if something would really break on Linux BE if we
> > > forget
> > about
> > > VRSAVE at all in the JVM. If not, we could forget about VRSAVE
> > > forever on
> > Linux.
> > > Looks like VRSAVE was sort of born to the oblivion... ?
> > >
> > >
> > > Kind regards,
> > > Gustavo
> > >
> > > [1] https://urldefense.proofpoint.com/v2/url?u=http-
> > 3A__refspecs.linuxfoundation.org_ELF_ppc64_PPC-2Delf64abi-
> > 2D1.9.html&d=DwIFAg&c=jf_iaSHvJObTbx-
> > siA1ZOg&r=kYUIMs9GlX4qSpgorcaHtHrNQxh38_XLwoS4XhaXum8&m=ih0Z-
> > esrs-
> > Hl9wipN392okVsz6z70Rsr9rgJinnzArY&s=arAjOio5NNoRIZLdczhgF5BDoAF3HU
> > vq-xCtSufn_kA&e=
> > > [2] https://urldefense.proofpoint.com/v2/url?u=http-
> > 3A__mail.openjdk.java.net_pipermail_ppc-2Daix-2Dport-2Ddev_2016-
> > 2DMay_002508.html&d=DwIFAg&c=jf_iaSHvJObTbx-
> > siA1ZOg&r=kYUIMs9GlX4qSpgorcaHtHrNQxh38_XLwoS4XhaXum8&m=ih0Z-
> > esrs-Hl9wipN392okVsz6z70Rsr9rgJinnzArY&s=p0xb08lxayJHBXZREL-7c5ipKc-
> > waZMMZpTiQWfU-S4&e=
> > >