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
