review request: 7184394: add intrinsics to use AES instructions
Christian Thalinger
christian.thalinger at oracle.com
Thu Aug 16 10:14:33 PDT 2012
On Aug 16, 2012, at 7:11 AM, Eric Caspole <eric.caspole at amd.com> wrote:
> 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/
Looks good. -- Chris
>
> 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