RFR: 8329035: New Data Destination instructions support [v2]
Sandhya Viswanathan
sviswanathan at openjdk.org
Wed Sep 4 23:53:57 UTC 2024
On Tue, 3 Sep 2024 22:22:56 GMT, Steve Dohrmann <sdohrmann at openjdk.org> wrote:
>> Adds assembler support for APX New Data Destination (NDD) and No Flags (NF) features.
>>
>> The NDD feature is supported by new functions that take an additional destination-only register operand. If the instruction also supports NF, a no_flags boolean parameter is present. To use these instructions with NF behavior, but without NDD semantics, the same register can be supplied for both the new destination and the (first) source operand.
>>
>> Some instructions support NF but not NDD. These instructions have a new function that just adds a boolean no_flags parameter. Existing functions were not overloaded with a boolean here because of signature collisions (bool / int) with functions that take immediate operands.
>>
>> All of the new functions have a letter "e" prefix, to avoid signature collisions and to indicate they will be evex encoded.
>
> Steve Dohrmann has updated the pull request incrementally with one additional commit since the last revision:
>
> function name changes based on review comments
src/hotspot/cpu/x86/assembler_x86.cpp line 1361:
> 1359: InstructionMark im(this);
> 1360: InstructionAttr attributes(AVX_128bit, /* vex_w */ false, /* legacy_mode */ false, /* no_mask_reg */ true, /* uses_vl */ false);
> 1361: attributes.set_address_attributes(/* tuple_type */ EVEX_NOSCALE, /* input_size_in_bits */ EVEX_64bit);
The input_size_in_bits could be EVEX_32bit.
src/hotspot/cpu/x86/assembler_x86.cpp line 1403:
> 1401: InstructionMark im(this);
> 1402: InstructionAttr attributes(AVX_128bit, /* vex_w */ false, /* legacy_mode */ false, /* no_mask_reg */ true, /* uses_vl */ false);
> 1403: attributes.set_address_attributes(/* tuple_type */ EVEX_NOSCALE, /* input_size_in_bits */ EVEX_64bit);
The input_size_in_bits could be EVEX_32bit.
src/hotspot/cpu/x86/assembler_x86.cpp line 1475:
> 1473: void Assembler::eaddb(Register dst, Register src, int imm8, bool no_flags) {
> 1474: InstructionAttr attributes(AVX_128bit, /* vex_w */ false, /* legacy_mode */ false, /* no_mask_reg */ true, /* uses_vl */ false);
> 1475: // (void) evex_prefix_and_encode_ndd(src->encoding(), dst->encoding(), 0, VEX_SIMD_NONE, VEX_OPCODE_0F_3C, &attributes);
Looks like the commented line is left over.
src/hotspot/cpu/x86/assembler_x86.cpp line 1780:
> 1778: void Assembler::eandw(Register dst, Register src1, Register src2, bool no_flags) {
> 1779: InstructionAttr attributes(AVX_128bit, /* vex_w */ false, /* legacy_mode */ false, /* no_mask_reg */ true, /* uses_vl */ false);
> 1780: (void) evex_prefix_and_encode_ndd(src1->encoding(), dst->encoding(), src2->encoding(), VEX_SIMD_NONE, VEX_OPCODE_0F_3C, &attributes, no_flags);
This should be VEX_SIMD_66 instead of VEX_SIMD_NONE.
src/hotspot/cpu/x86/assembler_x86.cpp line 1793:
> 1791: InstructionMark im(this);
> 1792: InstructionAttr attributes(AVX_128bit, /* vex_w */ false, /* legacy_mode */ false, /* no_mask_reg */ true, /* uses_vl */ false);
> 1793: attributes.set_address_attributes(/* tuple_type */ EVEX_NOSCALE, /* input_size_in_bits */ EVEX_64bit);
input_size_in_bits should be EVEX_32bit here.
src/hotspot/cpu/x86/assembler_x86.cpp line 1819:
> 1817: InstructionMark im(this);
> 1818: InstructionAttr attributes(AVX_128bit, /* vex_w */ false, /* legacy_mode */ false, /* no_mask_reg */ true, /* uses_vl */ false);
> 1819: attributes.set_address_attributes(/* tuple_type */ EVEX_NOSCALE, /* input_size_in_bits */ EVEX_64bit);
input_size_in_bits should EVEX_32bit.
src/hotspot/cpu/x86/assembler_x86.cpp line 1835:
> 1833: InstructionMark im(this);
> 1834: InstructionAttr attributes(AVX_128bit, /* vex_w */ false, /* legacy_mode */ false, /* no_mask_reg */ true, /* uses_vl */ false);
> 1835: attributes.set_address_attributes(/* tuple_type */ EVEX_NOSCALE, /* input_size_in_bits */ EVEX_64bit);
input_size_in_bits should EVEX_32bit.
src/hotspot/cpu/x86/assembler_x86.cpp line 1837:
> 1835: attributes.set_address_attributes(/* tuple_type */ EVEX_NOSCALE, /* input_size_in_bits */ EVEX_64bit);
> 1836: evex_prefix_ndd(src2, dst->encoding(), src1->encoding(), VEX_SIMD_NONE, VEX_OPCODE_0F_3C, &attributes, no_flags);
> 1837: emit_operand(src1, src2, 0);
emit_int8(0x23) is missing before call to emit_operand().
src/hotspot/cpu/x86/assembler_x86.cpp line 2015:
> 2013: void Assembler::ecmovl(Condition cc, Register dst, Register src1, Address src2) {
> 2014: InstructionMark im(this);
> 2015: NOT_LP64(guarantee(VM_Version::supports_cmov(), "illegal instruction"));
This assert could be removed.
src/hotspot/cpu/x86/assembler_x86.cpp line 2642:
> 2640:
> 2641: void Assembler::edecl(Register dst, Address src, bool no_flags) {
> 2642: // Don't use it directly. Use MacroAssembler::decrement() instead.
This comment could be removed.
src/hotspot/cpu/x86/assembler_x86.cpp line 2721:
> 2719: InstructionAttr attributes(AVX_128bit, /* vex_w */ false, /* legacy_mode */ false, /* no_mask_reg */ true, /* uses_vl */ false);
> 2720: int encode = evex_prefix_and_encode_nf(0, 0, src->encoding(), VEX_SIMD_NONE, VEX_OPCODE_0F_3C, &attributes, no_flags);
> 2721: emit_int16((unsigned char)0xA7, (0xE8 | encode));
Should this be 0xF7?
src/hotspot/cpu/x86/assembler_x86.cpp line 2988:
> 2986:
> 2987: void Assembler::elzcntl(Register dst, Register src, bool no_flags) {
> 2988: assert(VM_Version::supports_lzcnt(), "encoding is treated as BSR");
This assert could be removed.
src/hotspot/cpu/x86/assembler_x86.cpp line 3004:
> 3002:
> 3003: void Assembler::elzcntl(Register dst, Address src, bool no_flags) {
> 3004: assert(VM_Version::supports_lzcnt(), "encoding is treated as BSR");
This assert could be removed.
src/hotspot/cpu/x86/assembler_x86.cpp line 12751:
> 12749: }
> 12750: if (nds_is_ndd) attributes->set_extended_context();
> 12751: bool is_extended = adr.base_needs_rex2() || adr.index_needs_rex2() || nds_enc >= 16 || xreg_enc >= 16 || nds_is_ndd || force_evex;
If is_evex_instruction() is set for ndd and nf already at calling place as in my previous review comments, then is_extended could remain as before:
bool is_extended = adr.base_needs_rex2() || adr.index_needs_rex2() || nds_enc >= 16 || xreg_enc >= 16;
src/hotspot/cpu/x86/assembler_x86.cpp line 12841:
> 12839:
> 12840: clear_managed();
> 12841: if ((UseAVX > 2 && !attributes->is_legacy_mode()) || nds_is_ndd || force_evex)
If is_evex_instruction() is set for ndd and nf already at calling place as in my previous review comments, then this if could remain as before: if (UseAVX > 2 && !attributes->is_legacy_mode())
src/hotspot/cpu/x86/assembler_x86.hpp line 796:
> 794: void evex_prefix_ndd(Address adr, int ndd_enc, int xreg_enc, VexSimdPrefix pre, VexOpcode opc, InstructionAttr *attributes, bool no_flags = false) {
> 795: vex_prefix(adr, ndd_enc, xreg_enc, pre, opc, attributes, /* nds_is_ndd */ true , /* force_evex */ true, no_flags);
> 796: }
The additional parameter force_evex could be removed and the above could be encoded as:
attributes.set_is_evex_instruction();
vex_prefix(adr, ndd_enc, xreg_enc, pre, opc, attributes, /* nds_is_ndd */ true , no_flags);
src/hotspot/cpu/x86/assembler_x86.hpp line 800:
> 798: void evex_prefix_nf(Address adr, int ndd_enc, int xreg_enc, VexSimdPrefix pre, VexOpcode opc, InstructionAttr *attributes, bool no_flags = false) {
> 799: vex_prefix(adr, ndd_enc, xreg_enc, pre, opc, attributes, /* nds_is_ndd */ false , /* force_evex */ true, no_flags);
> 800: }
The additional parameter force_evex could be removed and the above could be encoded as:
attributes.set_is_evex_instruction();
vex_prefix(adr, ndd_enc, xreg_enc, pre, opc, attributes, /* nds_is_ndd */ false, no_flags);
src/hotspot/cpu/x86/assembler_x86.hpp line 811:
> 809: int evex_prefix_and_encode_ndd(int dst_enc, int nds_enc, int src_enc, VexSimdPrefix pre, VexOpcode opc,
> 810: InstructionAttr *attributes, bool no_flags = false) {
> 811: return vex_prefix_and_encode(dst_enc, nds_enc, src_enc, pre, opc, attributes, /* src_is_gpr */ true, /* nds_is_ndd */ true , /* force_evex */ true, no_flags);
The additional parameter force_evex could be removed and the above could be encoded as:
attributes.set_is_evex_instruction();
return vex_prefix_and_encode(dst_enc, nds_enc, src_enc, pre, opc, attributes, /* src_is_gpr */ true, /* nds_is_ndd */ true, no_flags);
src/hotspot/cpu/x86/assembler_x86.hpp line 816:
> 814: int evex_prefix_and_encode_nf(int dst_enc, int nds_enc, int src_enc, VexSimdPrefix pre, VexOpcode opc,
> 815: InstructionAttr *attributes, bool no_flags = false) {
> 816: return vex_prefix_and_encode(dst_enc, nds_enc, src_enc, pre, opc, attributes, /* src_is_gpr */ true, /* nds_is_ndd */ false, /* force_evex */ true, no_flags);
The additional parameter force_evex could be removed and the above could be encoded as:
attributes.set_is_evex_instruction();
return vex_prefix_and_encode(dst_enc, nds_enc, src_enc, pre, opc, attributes, /* src_is_gpr */ true, /* nds_is_ndd */ false, no_flags);
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744565879
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744593576
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744605475
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744611664
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744612351
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744613073
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744613754
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744614108
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744628195
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744616282
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744618760
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744627345
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744627491
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744241014
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744251493
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744234434
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744234962
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744237546
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1744238289
More information about the hotspot-compiler-dev
mailing list