[8u] RFR: 8143925 : enhancing CounterMode.crypt() for AESCrypt.implEncryptBlock() + 8146581 : Minor corrections to the patch submitted for earlier bug id - 8143925

[Anton Kozlov]

Hi,

On 16.01.2020 22:33, Martin Balao wrote:
> I'm not an official reviewer but I'll have a look at this, as I had my
> own 8u backport of 8143925 -which I did not propose due to lack of
> time-. I had a quick look and may have some comments. Please hold the push.
>
> http://cr.openjdk.java.net/~mbalao/webrevs/8143925/8143925.webrev.8u.hotspot.v0/

Thanks for publishing, I compared them and found some differences

> It worth noticing that this patch affects AArch64 too (for which I have
> a minor patch).

As well as other CPUs :) For example, we will make any required change for aarch32-port, but only after anything actually pushed to jdk8u.

> 
>  * src/cpu/x86/vm/assembler_x86.cpp
>   * 1st hook does not apply cleanly because 8210764 [2], 8132207 [3] and
> 8145688 [4] are not in 8u. Manually applied changes.
>   * 4th hook does not apply cleanly because 8132207 [3] is not in 8u.
> Manually applied changes.
>   * Removed all instruction attributes

Attributes in original patch hold rex_w parameter that is accepted by 8u's simd_prefix.
I think you should pass true it at least in pextrq and pinsrq.

>  * src/cpu/x86/vm/stubRoutines_x86_64.hpp
>   * 1st hook does not apply cleanly because 8132160 [6] is not in 8u.
> Manually applied changes adding 3000 to the current code_size2 value.
>
>  * src/cpu/x86/vm/stubRoutines_x86_32.hpp
>   * 1st hook does not apply cleanly because 8132160 [6] is not in 8u.
> Manually applied changes adding 3800 to the current code_size2 value.

Oh, great. In my patch, I've confused 8 with 0 and used 3000 here as well. Strange, it should crash right at the start, probably need a debug build. Thanks, I'll make an update.

>  * src/cpu/x86/vm/vm_version_x86.cpp
>   * 3rd hook does not apply cleanly because 8134553 [5] is not in 8u.
> Manually applied changes. Re-ordered the hooks and added conditionals to
> keep the semantics.

You created a separate if/else. Although this is IMHO cleaner than the original patch, I think it will create difficulties applying following JDK-8146581.

Similar minor comment: you fixed indentation issue in original patch [1]. The cosmetic difference in code doesn't affect any jdk8u user. So there is no reason to deviate from original code, making further backports more complicated.

[1]: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/72f54de44772#l12.53

>  * src/share/vm/opto/library_call.cpp
>   * 1st hook does not apply cleanly because 8073583 [8] is not in 8u.
>   * Added C param to CheckCastPPNode, ProjNode, CmpINode, BoolNode
> allocators

Compile::make_vm_intrinsic should declare non zero number of predicates for new intrinsic. Without this, inline_counterMode_AESCrypt_predicate will not be called.

> Testing
> 
>  * test/compiler/7184394/TestAESMain.java
>   * Passed

I would suggest observing actual performance improvement. The test passes with and without the intrinsic implementation.

Thanks,
Anton