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