On Tue, 8 Jun 2021 23:42:13 GMT, Sandhya Viswanathan <sviswanat...@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