Cool stuff.  I'm glad to see SPARC xmulx getting some play.

This is not a review, but I have a small and a big comment.

You don't need the argument vmIntrinsics::ID id unless you are going to do 
something with it, such as expand one of a family of intrinsics covered by the 
same LCK::inline* routine.

You probably don't have enough parameter verification on the inputs to 
processBlocks.  A robust range check for a subsequence indexing operation 
requires three comparison operations, not just one.  The Java code uses getLong 
which includes additional range checks, but your intrinsic replacement code 
might not do that correctly; in any case, it's better to have all the 
subsequence range checks in one place, and in Java code.  (Oh, and watch out 
for 32-bit integer overflow:  That causes surprises sometimes, when a value 
that is too large wraps to negative, and then can seem to be in range relative 
to a <= check.)

In fact, I don't believe the existing parameter verification is very helpful at 
all:

   if (inLen - inOfs > in.length)  throw...

This appears to allow a ridiculously large inLen as long as inOfs is also 
ridiculously large.  All of the "heavy lifting" is done by intrinisic array 
range checks triggered by getLong, and those are the checks that the assembly 
code does *not* do.

— John

P.S.  We should have a library API to do these parameter checks.  See:
  https://bugs.openjdk.java.net/browse/JDK-8042997#comment-13610181

On Feb 16, 2015, at 1:11 PM, Anthony Scarpino <anthony.scarp...@oracle.com> 
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.
> 
> 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

Reply via email to