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

[Martin Balao]

On 1/22/20 9:16 AM, Anton Kozlov wrote:
> 
> 
> 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.

I support all of your arguments here. Thanks for taking the time of such
a thoughtful analysis.

Thanks,
Martin.-