review request: 7184394: add intrinsics to use AES instructions
Eric Caspole
eric.caspole at amd.com
Thu Aug 16 07:11:00 PDT 2012
Hi Christian,
I think I addressed all your comments and fixed the funny spacing.
Here is the update:
http://cr.openjdk.java.net/~ecaspole/tom_aes/webrev.01/
Regards,
Eric
On Aug 13, 2012, at 6:22 PM, Christian Thalinger wrote:
>
> 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.
>>>
>>
>>
>
>
More information about the hotspot-compiler-dev
mailing list