RFR: 8268276: Base64 Decoding optimization for x86 using AVX-512 [v3]

[Scott Gibbons]

On Tue, 8 Jun 2021 23:42:13 GMT, Sandhya Viswanathan <sviswanathan at openjdk.org> wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fixing review comments.  Adding notes about isMIME parameter for other architectures; clarifying decodeBlock comments.
>
> src/hotspot/cpu/x86/assembler_x86.cpp line 4555:
> 
>> 4553: void Assembler::evpmaddubsw(XMMRegister dst, XMMRegister src1, XMMRegister src2, int vector_len) {
>> 4554:   assert(VM_Version::supports_avx512bw(), "");
>> 4555:   InstructionAttr attributes(vector_len, /* rex_w */ false, /* legacy_mode */ _legacy_mode_bw, /* no_mask_reg */ true, /* uses_vl */ true);
> 
> This instruction is also supported on AVX platforms. The assert check could be as follows:
>   assert(vector_len == AVX_128bit? VM_Version::supports_avx() :
>              vector_len == AVX_256bit? VM_Version::supports_avx2() :
>              vector_len == AVX_512bit? VM_Version::supports_avx512bw() : 0, "");
> Accordingly the instruction could be named as vpmaddubsw.

Done.

> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 5688:
> 
>> 5686:   address base64_vbmi_lookup_lo_addr() {
>> 5687:     __ align(64, (unsigned long) __ pc());
>> 5688:     StubCodeMark mark(this, "StubRoutines", "lookup_lo");
> 
> It will be good to add base64 to the StubCodeMark name for this and all the tables.

Done.

> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 5983:
> 
>> 5981:     // calculate length from offsets
>> 5982:     __ movq(length, end_offset);
>> 5983:     __ subq(length, start_offset);
> 
> These are 32bit, so movl, subl instead of movq, subq. Similar for all length relates instructions below.

Done.

> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 5987:
> 
>> 5985: 
>> 5986:     // If AVX512 VBMI not supported, just compile non-AVX code
>> 5987:     if(VM_Version::supports_avx512_vbmi()) {
> 
> Need to also check for VM_Version::supports_avx512bw() support.
> Could you please check if VM_Version::supports_avx512dq is needed as well?

Done. No need for avx512dq.

> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 6134:
> 
>> 6132:       __ subq(length, 64);
>> 6133:       __ addq(source, 64);
>> 6134:       __ addq(dest, 48);
> 
> All address related instructions here and below could use addptr, subptr etc.

Done.

> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 6273:
> 
>> 6271: 
>> 6272:     __ shrq(length, 2);    // Multiple of 4 bytes only - length is # 4-byte chunks
>> 6273:     __ cmpq(length, 0);
> 
> Should these be shrl, cmpl?

Done.

> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 6278:
> 
>> 6276:     // Set up src and dst pointers properly
>> 6277:     __ addq(source, start_offset);     // Initial offset
>> 6278:     __ addq(dest, dp);
> 
> The convention is to use addptr for pointers.

Done.

> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 6284:
> 
>> 6282:     __ shll(isURL, 8);    // index into decode table based on isURL
>> 6283:     __ lea(decode_table, ExternalAddress(StubRoutines::x86::base64_decoding_table_addr()));
>> 6284:     __ addq(decode_table, isURL);
> 
> addptr here.

Done.

> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 6297:
> 
>> 6295:     __ orl(byte1, byte4);
>> 6296: 
>> 6297:     __ incrementq(source, 4);
> 
> addptr here.

Done.

> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 6317:
> 
>> 6315:     __ load_signed_byte(byte4, Address(source, RegisterOrConstant(), Address::times_1, 3));
>> 6316:     __ load_signed_byte(byte3, Address(decode_table, byte3, Address::times_1, 0));
>> 6317:     __ load_signed_byte(byte4, Address(decode_table, byte4, Address::times_1, 0));
> 
> You could use Address(base, offset) form directly here and other places: e.g. Address (source, 1) instead of Address(source, RegisterOrConstant(), Address::times_1, 1).

Done.

> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 6329:
> 
>> 6327:     __ subq(dest, rax);      // Number of bytes converted
>> 6328:     __ movq(rax, dest);
>> 6329:     __ pop(rbx);
> 
> subptr, movptr here.

Done.

> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 7627:
> 
>> 7625:       StubRoutines::x86::_right_shift_mask = base64_right_shift_mask_addr();
>> 7626:       StubRoutines::_base64_encodeBlock = generate_base64_encodeBlock();
>> 7627:       if (VM_Version::supports_avx512_vbmi()) {
> 
> Need to add avx512bw check here also.

Done.

> src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 7628:
> 
>> 7626:       StubRoutines::_base64_encodeBlock = generate_base64_encodeBlock();
>> 7627:       if (VM_Version::supports_avx512_vbmi()) {
>> 7628:         StubRoutines::x86::_lookup_lo = base64_vbmi_lookup_lo_addr();
> 
> It would be good to add base64 to these names.

Done.

-------------

PR: https://git.openjdk.java.net/jdk/pull/4368