RFR: 8073108: GHASH Intrinsics

[Anthony Scarpino]

On 02/16/2015 06:47 PM, John Rose wrote:
> 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.

Sure.. In my following the AES intrinsics as an example, I didn't catch 
'id' pointlessness..

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

Ok.. thanks.. I'll double check the range checking code..

>
> — 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.scarpino at 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
>