review request: 7184394: add intrinsics to use AES instructions

[Christian Thalinger]

On Aug 13, 2012, at 10:15 AM, Eric Caspole <eric.caspole at amd.com> wrote:

> Hi Roland and Vladimir,
> Tom is on vacation for another week so I updated the AES webrev code according to your comments, so we don't miss out on any deadlines or testing opportunities.
> 
> http://cr.openjdk.java.net/~ecaspole/tom_aes/webrev/

src/cpu/x86/vm/stubGenerator_x86_32.cpp:

+  void loadKey(XMMRegister xmmdst, Register key, int ofst, XMMRegister xmm_shuf_mask=NULL) {
+  void aesencKey(XMMRegister xmmdst, XMMRegister xmmtmp, Register key, int ofst, XMMRegister xmm_shuf_mask=NULL) {
+  void aesdecKey(XMMRegister xmmdst, XMMRegister xmmtmp, Register key, int ofst, XMMRegister xmm_shuf_mask=NULL) {

Can we use non-camel case names for these methods?

+    for (int ofst = 0x10; ofst <= 0x90; ofst += 0x10) {

I guess ofst means offset.  If so, could we rename it?

+    __ jcc(Assembler::notEqual,L_singleBlock_loopTop_192); 
+    for (int rnum = XMM_REG_NUM_KEY_FIRST + 1; rnum  <= XMM_REG_NUM_KEY_LAST; rnum++) {

Spacing is sometimes odd (no space, two spaces).

src/cpu/x86/vm/vm_version_x86.cpp:

 474   // Use AES instructions if available.
 475   if (supports_aes()) {
 476     if (FLAG_IS_DEFAULT(UseAES)) {
 477       UseAES = true;
 478     }
 479   } else if (UseAES) {
 480           if (!FLAG_IS_DEFAULT(UseAES))
 481             warning("AES instructions not available on this CPU");
 482           FLAG_SET_DEFAULT(UseAES, false);
 483   }
 484 
 485   // The AES intrinsic stubs require AES instruction support (of course) 
 486   // but also require AVX mode for misaligned SSE access
 487   if (UseAES && (UseAVX > 0)) {
 488     if (FLAG_IS_DEFAULT(UseAESIntrinsics)) {
 489       UseAESIntrinsics = true;
 490     }
 491   } else if (UseAESIntrinsics) {
 492           if (!FLAG_IS_DEFAULT(UseAESIntrinsics))
 493             warning("AES intrinsics not available on this CPU");
 494     FLAG_SET_DEFAULT(UseAESIntrinsics, false);
 495   }
 496 

The indentation is off.

src/share/vm/classfile/vmSymbols.hpp:

 722   /* support for com.sum.crypto.provider.AESCrypt and some of its callers */                                            \
 723   do_class(com_sun_crypto_provider_aescrypt,      "com/sun/crypto/provider/AESCrypt")                                   \
 724   do_intrinsic(_aescrypt_encryptBlock, com_sun_crypto_provider_aescrypt, encryptBlock_name, byteArray_int_byteArray_int_signature, F_R)   \
 725   do_intrinsic(_aescrypt_decryptBlock, com_sun_crypto_provider_aescrypt, decryptBlock_name, byteArray_int_byteArray_int_signature, F_R)   \
 726    do_name(     encryptBlock_name,                                 "encryptBlock")                                      \
 727    do_name(     decryptBlock_name,                                 "decryptBlock")                                      \
 728    do_signature(byteArray_int_byteArray_int_signature,                  "([BI[BI)V")                                    \
 729                                                                                                                         \
 730   do_class(com_sun_crypto_provider_cipherBlockChaining,      "com/sun/crypto/provider/CipherBlockChaining")             \
 731    do_intrinsic(_cipherBlockChaining_encryptAESCrypt, com_sun_crypto_provider_cipherBlockChaining, encrypt_name, byteArray_int_int_byteArray_int_signature, F_R)   \
 732    do_intrinsic(_cipherBlockChaining_decryptAESCrypt, com_sun_crypto_provider_cipherBlockChaining, decrypt_name, byteArray_int_int_byteArray_int_signature, F_R)   \
 733     do_name(     encrypt_name,                                                "encrypt")                                \
 734     do_name(     decrypt_name,                                                "decrypt")                                \
 735     do_signature(byteArray_int_int_byteArray_int_signature,             "([BII[BI)V")                                   \

Same here.  We try to keep them aligned for better readability:

src/share/vm/oops/methodOop.cpp:

+      strcmp(instanceKlass::cast(holder)->class_loader()->klass()->klass_part()->external_name(), "sun.misc.Launcher$ExtClassLoader") != 0)

I think we should define a vmSymbol for the class loader name and do a pointer compare.  I don't like the strcmp here.

src/share/vm/opto/escape.cpp:

+            char str[200];
+            sprintf(str, "EA: unexpected CallLeaf %s", call->as_CallLeaf()->_name);
+            assert(false, str);

You should use err_msg_res and guarantee here like:

+            guarantee(err_msg_res("EA: unexpected CallLeaf %s", call->as_CallLeaf()->_name));

Otherwise this looks good.

-- Chris

> 
> Let us know what you think.
> Thanks,
> Eric
> 
> On Aug 9, 2012, at 2:00 PM, Roland Westrelin wrote:
> 
>> 
>> Hi Tom,
>> 
>>> http://cr.openjdk.java.net/~tdeneau/aes-intrinsics/webrev.04
>>> 
>>> has been posted which addresses the issues that Vladimir raised.
>> 
>> In callGenerator.cpp, this should go away:
>> 
>> 966   // Vladimir said to remove this check
>> 967   if (false && kit.stopped()) {
>> 968     // Receiver's null check or other exception.
>> 969     return kit.transfer_exceptions_into_jvms();
>> 970   }
>> 
>> In library_call.cpp, typos:
>> 
>> 5784 // the psuedo code we want to emulate with this predicate is:
>> 
>> 5811   // we want to an instanceof comparison against the AESCrypt class
>> 
>> Also, it looks like this should go away as well:
>> 
>> 5840   if (false) {
>> 5841     //  src and dest are arrays.
>> 5842     const Type* src_type = src->Value(&_gvn);
>> 5843     const Type* dest_type = dest->Value(&_gvn);
>> 5844     const TypeAryPtr* top_src = src_type->isa_aryptr();
>> 5845     const TypeAryPtr* top_dest = dest_type->isa_aryptr();
>> 5846     assert (top_src  != NULL && top_src->klass()  != NULL
>> 5847             &&  top_dest != NULL && top_dest->klass() != NULL, "args are strange");
>> 5848   }
>> 
>> Otherwise, I think it'ok to me.
>> 
>> Roland.
>> 
> 
>