RFR (L): 7088419 : Use x86 Hardware CRC32 Instruction with java.util.zip.CRC32 and java.util.zip.Adler32

[Christian Thalinger]

Looks good.  -- Chris

On Jun 28, 2013, at 6:36 AM, David Chase <david.r.chase at oracle.com> wrote:

> New webrev, allegedly addresses issues below:
> 
> http://cr.openjdk.java.net/~drchase/7088419/webrev.06/
> 
> On 2013-06-26, at 2:17 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
> 
>> For Doxygen parameters you should use @param:
>> 
>> http://www.stack.nl/~dimitri/doxygen/manual/commands.html#cmdparam
> 
> Added @param, I think (following idioms at website listed, a bit of a problem since
> the fact that it is code generation means that it's all one level removed from what is
> really happening.  You might want to eyeball these.
> 
>> src/share/vm/prims/jvm.cpp:
>> 
>> +   PUTPROP(props, "sun.zip.clmulSupported", X86_ONLY(!UseCRC32Intrinsics && UseAVX && UseCLMUL ? "true" :) "false");
> 
> Gone.
> 
>> src/cpu/x86/vm/templateInterpreter_x86_32.cpp:
>> 
>> I thought I've changed this already but apparently I didn't:
>> 
>>     case Interpreter::java_lang_math_exp     : entry_point = ((InterpreterGenerator*)this)->generate_math_entry(kind);     break;
>> 
>> Can we please add a local variable for (InterpreterGenerator*)this (on all architectures)?
> 
> ig_this, at least on the two intel architectures.
> 
>> src/share/vm/opto/library_call.cpp:
>> 
>> +   crc = _gvn.transform( new (C) URShiftINode(crc, intcon(8)) );
>> +   result = _gvn.transform( new (C) XorINode(crc, result) );
>> +   result = _gvn.transform( new (C) XorINode(result, M1) );
>> 
>> Can we get rid of the spaces after ( and before )?  It's probably from copy-paste.
> 
> 1,$s/( new/(new/g
> 1,$s/) );/));/g
> 
>> About test:
>> 
>> '@summary' should bug synopsis: "Use x86 Hardware CRC32 Instruction with java.util.zip.CRC32 and java.util.zip.Adler32".
>> 
>> What test does is a separate comment.
>> And you forgot '@run' command:
>> 
>> * @run main CRCTest
> 
> 
> And also fixed the test.
> 
> Ran test locally with jtreg, ran just that test w/ jprt across the relevant suspects (Mac, Linux, Solaris, Windows) x (Intel 32, Intel 64, Sparc).
> 
> David
>