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

[Anton Kozlov]

On 22.01.2020 04:11, Hohensee, Paul wrote:
> I'm not a huge fan of making manual changes to get around not having backported patches on which a patch (i.e., this one) depends (whew!). I'd rather see the 8 (!) patches Martin references done first. 

Are you sure the list of 8 patches do not pull other ones, and so on?
Let's review the list:

> [1] - https://bugs.openjdk.java.net/browse/JDK-8143925
-- the bug itself, ignore

> [2] - https://bugs.openjdk.java.net/browse/JDK-8210764
JDK-8210764: Update avx512 implementation

> [3] - https://bugs.openjdk.java.net/browse/JDK-8132207
JDK-8132207: update for x86 exp in the math lib

> [4] - https://bugs.openjdk.java.net/browse/JDK-8145688
JDK-8145688: update for x86 pow in the math lib

> [5] - https://bugs.openjdk.java.net/browse/JDK-8134553
JDK-8134553: CRC32C implementations for x86/x64 targets

> [6] - https://bugs.openjdk.java.net/browse/JDK-8132160
JDK-8132160: support for AVX 512 call frames and stack management

> [7] - https://bugs.openjdk.java.net/browse/JDK-8130832
JDK-8130832: Extend the WhiteBox API to provide information about the availability of compiler intrinsics

> [8] - https://bugs.openjdk.java.net/browse/JDK-8073583
JDK-8073583: C2 support for CRC32C on SPARC

Group them:

>  * 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.

Patches 2, 3, 4, 6 fix AVX512 related code. It's not implemented at all in jdk8u. Do you suggest to backport it? 
We'll need to fight extremely complex x86 SIMD encoding and test this in various environments. 
And then we still be needed in careful code-review, like one from Intel-people who wrote the code and from Hotspot-gurus who reviewed original integration to jdk9.
Sounds like jdk8u-avx512-incubator :)


>  * 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
>
>  * 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.

5 and 8 implement CRC32C intrinsic support. Efforts for backport of them are comparable to this patch. 
It seems too excessive to do another intrinsic only to get rid of conflicts in infrastructure code and platform feature detection.

>  * src/share/vm/classfile/vmSymbols.cpp
>   * 1st and 2nd hook do not apply cleanly because 8130832 [7] is not in
> 8u. These changes are a N/A for 8u.
> 
>  * src/share/vm/opto/c2compiler.cpp
>   * 1st hook does not apply cleanly because 8130832 [7] is not in 8u.
> This change is an N/A for 8u.

7 is WhiteBox API change, the API is not used by this patch.


> If we don't, then if we/Oracle ever decide to do any of those backports, it's going to be very confusing.

If someone decide to backport AVX512, they will be required to do significant amount of work, that's true. 
I hope we'll not decide it without proper consideration and without strong reason.
Oracle did the RFRed patch and not a one from the list. We and they will be in same position if this patch applied.

> I'd rather have something as close as possible to what was tested in the parent repo.
IMHO. Pulling random changes because of formal (not semantic) dependency doesn't make us closer.
But it makes us further from stable 8u, that is tested even more. 
We tend to break things with this approach.

Considering formal dependency between patches is a good rule of thumb, but the dependency should not be blindly resolved. 
Note, Martin and I both omitted applying changes from the list, and I think it's good approach here.

Thanks,
Anton