RFR: 8329035: New Data Destination instructions support [v3]

[Jatin Bhateja]

On Fri, 6 Sep 2024 21:44:44 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:
> 
>   refactoring and fixes based on review comments

Hi @steveatgh , Reviewing your patch is a nice refresher of almost all x86 scalar instructions :-)
Have some minor comments.

src/hotspot/cpu/x86/assembler_x86.cpp line 1420:

> 1418:   emit_arith(0x11, 0xC0, src1, src2);
> 1419: }
> 1420: 

Encodings for ADC looks good, but I could not find uses for these instructions beyond stubs. Do you think we should add them lazily on need basis.

src/hotspot/cpu/x86/assembler_x86.cpp line 1452:

> 1450:   emit_int8(imm8);
> 1451: }
> 1452: 

Again I could not find any use of scalar 16 bit addition and only one use of 8 bit add. Should be consider adding them lazily.

src/hotspot/cpu/x86/assembler_x86.cpp line 1783:

> 1781:   emit_arith(0x23, 0xC0, src1, src2);
> 1782: }
> 1783: 

We can skip adding 8bit and 16bit flavors of AND instructions since they are not being used by compiler / stubs.

src/hotspot/cpu/x86/assembler_x86.cpp line 1795:

> 1793:   attributes.set_address_attributes(/* tuple_type */ EVEX_NOSCALE, /* input_size_in_bits */ EVEX_32bit);
> 1794:   evex_prefix_ndd(src, dst->encoding(), 0, VEX_SIMD_NONE, VEX_OPCODE_0F_3C, &attributes, no_flags);
> 1795:   emit_arith_operand(0x81, as_Register(4), src, imm32);

Suggestion:

  emit_arith_operand(0x81, rsp, src, imm32);

src/hotspot/cpu/x86/assembler_x86.cpp line 1840:

> 1838:   emit_operand(src1, src2, 0);
> 1839: }
> 1840: 

Should we only keep one _eandl (REG, REG, ADR)_ and remove other one _eandl(REG, ADR, REG)_, apart from source operand encodings there is no other difference b/w the two instructions.

src/hotspot/cpu/x86/assembler_x86.cpp line 2016:

> 2014: void Assembler::ecmovl(Condition cc, Register dst, Register src1, Address src2) {
> 2015:   InstructionMark im(this);
> 2016:   NOT_LP64(guarantee(VM_Version::supports_cmov(), "illegal instruction"));

APX is only supported by 64 bit (IA-32e mode) targets.

src/hotspot/cpu/x86/assembler_x86.cpp line 2696:

> 2694: }
> 2695: 
> 2696: void Assembler::eidivl(Register src, bool no_flags) { // Unsigned

Comment should be signed division.

src/hotspot/cpu/x86/assembler_x86.cpp line 6801:

> 6799:   emit_arith_operand(0x81, rbx, src, imm32);
> 6800: }
> 6801: 

Don't find any usage of SUB with borrow in existing code.  I think we can defer adding it till we have a use.

src/hotspot/cpu/x86/assembler_x86.cpp line 12723:

> 12721:   byte3 |= pre;
> 12722: 
> 12723:   // P2: byte 4 as zL'Lbv'aaa or 00L0VF00 where V = V4 and F = NF (no flags)

Suggestion:

  // P2: byte 4 as zL'Lbv'aaa or 00LXVF00 where V = V4, X(extended context) = ND and F = NF (no flags)

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

PR Review: https://git.openjdk.org/jdk/pull/20698#pullrequestreview-2291465334
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1751415531
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1751419391
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1751475124
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1751491312
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1751552473
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1751565632
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1751596776
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1751710213
PR Review Comment: https://git.openjdk.org/jdk/pull/20698#discussion_r1751327543