RFR(S) JDK-8205528: Base64 Encode Algorithm using AVX512 Instructions

[Kamath, Smita]

Please see my answers inline in your email below.

Thanks,
Smita

-----Original Message-----
From: Paul Sandoz [mailto:paul.sandoz at oracle.com] 
Sent: Friday, June 22, 2018 2:17 PM
To: Vladimir Kozlov <vladimir.kozlov at oracle.com>
Cc: Kamath, Smita <smita.kamath at intel.com>; hotspot compiler <hotspot-compiler-dev at openjdk.java.net>
Subject: Re: RFR(S) JDK-8205528: Base64 Encode Algorithm using AVX512 Instructions



> On Jun 22, 2018, at 12:29 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
> 
> Hi Smita,
> 
> I CCing to Libs to review code changes in Base64.java.
> 
> Looks like you changed all need place to implement intrinsic.
> One question so: why you have own copy of base64 charsets and not using one in library:
> 
>         private int encode0(byte[] src, int off, int end, byte[] dst) {
>             char[] base64 = isURL ? toBase64URL : toBase64;
> 

Yes, especially if we converted those from char[] to byte[] (which might also improve the C2 generated code) and pass the selected byte[] to the intrinsic. 
Smita>>  I need an integer array in order to use vpgatherdd instruction with vector index. Vpgather instruction works on 32 bit array and so I need to define base64 charset in a 32 bit array.

Naming wise for the Java methods here are some suggestions:

  generateImplEncode -> encodeBlockWithBoundsCheck
  implEncode -> encodeBlock

Also can generateImplEncode be private?
Smita>> I'll make these changes and send an updated webrev.
 
Further. is there is a need to perform bounds checks in generateImplEncode given the public methods calling encode will, i presume, have dominating checks?
Smita>> The check is not required. I'll retain encodeBlock and remove encodeBlockWithBoundsCheck. 

Paul.


> Some indents in new and old Assembler::emit_operand() are off.
> 
> In new Assembler::emit_operand() is better use } else { instead of 'return' in one branch.
> 
> This is first time I see that XMM register can be used for index in address. Is it true? Can you point to Intel's document which describes it.
> 
> What testing you did?
> 
> Please, add tests to test/hotspot/jtreg/compiler/intrinsics/base64 (may be similar to sha or AES in compiler/codegen/aes) to make sure that all flags, intrinsic is used and it produces correct result.
> 
> I know there is test/jdk/java/util/Base64/ tests but they may not trigger intrinsic usage. But you can use them as starting point for new tests.
> 
> Thanks,
> Vladimir
> 
> On 6/22/18 11:47 AM, Kamath, Smita wrote:
>> Hi Vladimir,
>> I’d like to contribute an optimization for Base64 Encoding Algorithm using AVX512 Instructions. This optimization shows 1.5x improvement on x86_64 platform(SKL).
>> Link to Bug: https://bugs.openjdk.java.net/browse/JDK-8205528
>> Link to webrev: http://cr.openjdk.java.net/~vdeshpande/Base64/webrev.00/
>> For testing the implementation, I have run tests under test/jdk/java/util/Base64/ and also ran test/jdk/com/sun/jndi/ldap/Base64Test.java
>> I have also run jtreg.
>> Please review and let me know if you have any comments.
>> Thanks and Regards,
>> Smita