Re: [PATCH V2] Support APX NF

[Jan Beulich <jbeulich@suse.com>]

On 04.03.2024 09:15, Cui, Lili wrote:
> gas/ChangeLog:
> 
>        * NEWS: Support Intel APX NF.
>        * config/tc-i386.c (enum i386_error): Add unsupported_nf.
>        (struct _i386_insn): Add has_nf.
>        (is_apx_evex_encoding): Ditto.
>        (build_apx_evex_prefix): Encode the NF bit.
>        (md_assemble): Handle unsupported_nf.
>        (parse_insn): Handle Prefix_NF.
>        (can_convert_NDD_to_legacy):
>        (match_template): Support D for APX_F insns.
>        * testsuite/gas/i386/x86-64-apx-evex-promoted-bad.d: Add bad test for NF bit.
>        * testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s: Ditto.
>        * testsuite/gas/i386/x86-64-apx-inval.l: Ditto.
>        * testsuite/gas/i386/x86-64-apx-inval.s: Ditto.
>        * testsuite/gas/i386/x86-64.exp: Add apx nf tests.
>        * testsuite/gas/i386/x86-64-apx-nf-intel.d: New test.
>        * testsuite/gas/i386/x86-64-apx-nf.d: Ditto.
>        * testsuite/gas/i386/x86-64-apx-nf.s: Ditto.
> 
> opcodes/ChangeLog:
> 
>        * i386-dis-evex.h: Add %XN to the instructions that support APX NF and
>        add new instruction imul, popcnt, tzcnt and lzcnt to EVEX table.
>        * i386-dis-evex-reg.h: Ditto.
>        * i386-dis.c (struct instr_info): Add nf.
>        (struct dis386): Add "NF" for EVEX.NF.
>        (get_valid_dis386): Set ins->vex.nf.
>        (print_insn): Handle ins.vex.nf.
>        (putop): Handle "%NF".
>        * i386-opc.h (Prefix_NoOptimize): Adjust the value.

Stale entry? As asked for before: If you provide this information, it needs
to be adjusted as patch contents actually changes.

>        (Prefix_NF): New.
>        * i386-opc.tbl: Add new entries for the instructions that support APX NF.

... plus a few that don't (while others that don't and aren't of use except
when used with {evex} were added prematurely).

> @@ -6655,6 +6663,9 @@ md_assemble (char *line)
>  	case unsupported_EGPR_for_addressing:
>  	  err_msg = _("extended GPR cannot be used as base/index");
>  	  break;
> +	case unsupported_nf:
> +	  err_msg = _("unsupported NF");
> +	  break;

"{nf} unsupported" (yielding an overall better complete diagnostic)?

> @@ -7218,6 +7229,11 @@ parse_insn (const char *line, char *mnemonic, bool prefix_only)
>  		  /* {rex2} */
>  		  i.rex2_encoding = true;
>  		  break;
> +		case Prefix_NF:
> +		  /* {nf} */
> +		  i.has_nf = true;
> +		  i.encoding = encoding_evex;
> +		  break;

It's not quite as easy, I'm afraid: Have you thought of the "{vex} {nf} ..."
case? (I think I previously indicated that their combination, actually in
either order, needs properly rejecting.) Without having spent much thought
on it, perhaps it would suffice to check here that the field is still
encoding_default, and leave the value alone otherwise (in order to reject
bad combinations elsewhere).

> --- a/gas/testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s
> +++ b/gas/testsuite/gas/i386/x86-64-apx-evex-promoted-bad.s
> @@ -23,7 +23,7 @@ _start:
>  	.insn EVEX.L1.66.M12.W0 0x60, %di, %ax
>  
>  	#EVEX_MAP4 movbe %r18w,%ax set EVEX.z == 0b1.
> -	.insn EVEX.L0.66.M12.W0 0x60, %di, %ax {%k7}{z}
> +	.insn EVEX.L0.66.M12.W0 0x60, %di, %ax {%k3}{z}

What's going on here ...

> @@ -33,17 +33,25 @@ _start:
>  	.insn EVEX.L1.NP.0f38.W1 0xf5, %rax, (%rax,%rbx), %rcx
>  
>  	#EVEX from VEX bzhi %rax,(%rax,%rbx),%rcx EVEX.P[23](EVEX.z) == 0b1
> -	.insn EVEX.L0.NP.0f38.W1 0xf5, %rax, (%rax,%rbx), %rcx {%k7}{z}
> +	.insn EVEX.L0.NP.0f38.W1 0xf5, %rax, (%rax,%rbx), %rcx {%k3}{z}

... and here? This is the kind of thing that wants mentioning in the
patch description. And once again, especially for larger patches it is
generally a bad idea if they come with only legacy ChangeLog entries:
These only say "what", never "why". (That said, I think I see why the
change is needed.)

Additionally may I ask that you get used to providing a brief revision
log when sending new versions of earlier patches? This helps reviewers
when they did look at the earlier version(s).

>  	#EVEX from VEX bzhi %rax,(%rax,%rbx),%rcx EVEX.P[20](EVEX.b) == 0b1
>  	.insn EVEX.L0.NP.0f38.W1 0xf5, %rax, (%rax,%rbx){1to8}, %rcx
>  
>  	#{evex} inc %rax %rbx EVEX.vvvv != 1111 && EVEX.ND = 0.
>  	.byte 0x62, 0xf4, 0xe4, 0x08, 0xff, 0x04, 0x08
> +
>  	# pop2 %rax, %r8 set EVEX.ND=0.
>  	.byte 0x62, 0xf4, 0x3c, 0x08, 0x8f, 0xc0
>  	.byte 0xff, 0xff, 0xff
> +
>  	# pop2 %rax, %r8 set EVEX.vvvv = 1111.
>  	.insn EVEX.L0.M4.W0 0x8f,  %rax, {rn-sae},%r8
> +
>  	# pop2 %r8, %r8.
>  	.byte 0x62, 0xd4, 0x3c, 0x18, 0x8f, 0xc0
> +	.byte 0xff, 0xff

As indicated - this would better be avoided by picking a more suitable
ModR/M byte on the earlier line. Using %r11 instead of %r8 ought to do
just fine here, I suppose.

> +	#EVEX_MAP4 movbe %r18w,%ax set EVEX.nf = 1.
> +        .insn EVEX.L0.66.M12.W0 0x60, %di, %ax {%k4}

Nit: Inconsistent indentation.

> --- /dev/null
> +++ b/gas/testsuite/gas/i386/x86-64-apx-nf.s
>[...]
> +	{nf}	rorq	%cl, 291(%r8, %rax, 4)
> +	{nf}	ror	%cl, 291(%r8, %rax, 4), %r9
> +	{nf}	sar	$1, %bl
> +	{nf}	sar	$1, %bl, %dl
> +	{nf}	sar	$1, %dx
> +	{nf}	sar	$1, %dx, %ax
> +	{nf}	sar	$1, %ecx
> +	{nf}	sar	$1, %ecx, %edx
> +	{nf}	sar	$1, %r9
> +	{nf}	sar	$1, %r9, %r31
> +	{nf}	sarb	$1, 291(%r8, %rax, 4)
> +	{nf}	sar	$1, 291(%r8, %rax, 4), %bl
> +	{nf}	sarw	$1, 291(%r8, %rax, 4)
> +	{nf}	sar	$1, 291(%r8, %rax, 4), %dx
> +	{nf}	sarl	$1, 291(%r8, %rax, 4)
> +	{nf}	sar	$1, 291(%r8, %rax, 4), %ecx
> +	{nf}	sarq	$1, 291(%r8, %rax, 4)
> +	{nf}	sar	$1, 291(%r8, %rax, 4), %r9
> +	{nf}	sar	$123, %bl
> +	{nf}	sar	$123, %bl, %dl
> +	{nf}	sar	$123, %dx
> +	{nf}	sar	$123, %dx, %ax
> +	{nf}	sar	$123, %ecx
> +	{nf}	sar	$123, %ecx, %edx
> +	{nf}	sar	$123, %r9
> +	{nf}	sar	$123, %r9, %r31
> +	{nf}	sarb	$123, 291(%r8, %rax, 4)
> +	{nf}	sar	$123, 291(%r8, %rax, 4), %bl
> +	{nf}	sarw	$123, 291(%r8, %rax, 4)
> +	{nf}	sar	$123, 291(%r8, %rax, 4), %dx
> +	{nf}	sarl	$123, 291(%r8, %rax, 4)
> +	{nf}	sar	$123, 291(%r8, %rax, 4), %ecx
> +	{nf}	sarq	$123, 291(%r8, %rax, 4)
> +	{nf}	sar	$123, 291(%r8, %rax, 4), %r9
> +	{nf}	sar	%cl, %bl
> +	{nf}	sar	%cl, %bl, %dl
> +	{nf}	sar	%cl, %dx
> +	{nf}	sar	%cl, %dx, %ax
> +	{nf}	sar	%cl, %ecx
> +	{nf}	sar	%cl, %ecx, %edx
> +	{nf}	sar	%cl, %r9
> +	{nf}	sar	%cl, %r9, %r31
> +	{nf}	sarb	%cl, 291(%r8, %rax, 4)
> +	{nf}	sar	%cl, 291(%r8, %rax, 4), %bl
> +	{nf}	sarw	%cl, 291(%r8, %rax, 4)
> +	{nf}	sar	%cl, 291(%r8, %rax, 4), %dx
> +	{nf}	sarl	%cl, 291(%r8, %rax, 4)
> +	{nf}	sar	%cl, 291(%r8, %rax, 4), %ecx
> +	{nf}	sarq	%cl, 291(%r8, %rax, 4)
> +	{nf}	sar	%cl, 291(%r8, %rax, 4), %r9
> +	{nf}	sal	$1, %bl
> +	{nf}	sal	$1, %bl, %dl

Hmm "sal" sorts ahead of "sar", doesn't it?

> --- a/opcodes/i386-dis-evex-reg.h
> +++ b/opcodes/i386-dis-evex-reg.h
> @@ -51,33 +51,33 @@
>    },
>    /* REG_EVEX_MAP4_80 */
>    {
> -    { "addA",	{ VexGb, Eb, Ib }, NO_PREFIX },
> -    { "orA",	{ VexGb, Eb, Ib }, NO_PREFIX },
> +    { "%NFaddA",	{ VexGb, Eb, Ib }, NO_PREFIX },
> +    { "%NForA",	{ VexGb, Eb, Ib }, NO_PREFIX },
>      { "adcA",	{ VexGb, Eb, Ib }, NO_PREFIX },
>      { "sbbA",	{ VexGb, Eb, Ib }, NO_PREFIX },

You (correctly) leave these alone.

> @@ -87,25 +87,33 @@
>    {
>      { Bad_Opcode },
>      { Bad_Opcode },
> -    { "notA",	{ VexGb, Eb }, NO_PREFIX },
> -    { "negA",	{ VexGb, Eb }, NO_PREFIX },
> +    { "%NFnotA",	{ VexGb, Eb }, NO_PREFIX },

Why does this gain %NF then?

> @@ -1815,6 +1817,8 @@ struct dis386 {
>     "XV" => print "{vex} " pseudo prefix
>     "XE" => print "{evex} " pseudo prefix if no EVEX-specific functionality is
>  	   is used by an EVEX-encoded (AVX512VL) instruction.
> +   "NF" => print "{nf} " pseudo prefix when EVEX.NF = 1.
> +	   a valid encoding.

What is this 2nd line, not even a complete sentence, supposed to be
saying?

> @@ -2619,25 +2623,25 @@ static const struct dis386 reg_table[][8] = {
>    },
>    /* REG_C0 */
>    {
> -    { "rolA",	{ VexGb, Eb, Ib }, NO_PREFIX },
> -    { "rorA",	{ VexGb, Eb, Ib }, NO_PREFIX },
> -    { "rclA",	{ VexGb, Eb, Ib }, NO_PREFIX },
> -    { "rcrA",	{ VexGb, Eb, Ib }, NO_PREFIX },
> -    { "shlA",	{ VexGb, Eb, Ib }, NO_PREFIX },
> -    { "shrA",	{ VexGb, Eb, Ib }, NO_PREFIX },
> -    { "shlA",	{ VexGb, Eb, Ib }, NO_PREFIX },
> -    { "sarA",	{ VexGb, Eb, Ib }, NO_PREFIX },
> +    { "%NFrolA",	{ VexGb, Eb, Ib }, NO_PREFIX },
> +    { "%NFrorA",	{ VexGb, Eb, Ib }, NO_PREFIX },
> +    { "%NFrclA",	{ VexGb, Eb, Ib }, NO_PREFIX },
> +    { "%NFrcrA",	{ VexGb, Eb, Ib }, NO_PREFIX },

Like NOT, RC{L,R} look to be wrongly being modified here. Interestingly
for D0...D3 you leave them alone.

> @@ -9147,6 +9151,10 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
>        ins->vex.v = *ins->codep & 0x8;
>        ins->vex.mask_register_specifier = *ins->codep & 0x7;
>        ins->vex.zeroing = *ins->codep & 0x80;
> +      /* Set the NF bit for the EVEX instruction extended from the legacy or
> +	 vex instruction, this bit will be cleared when it can be confirmed
> +	 that its defaut type is evex.  */
> +      ins->vex.nf = *ins->codep & 0x4;

Whow or what is "its" in the comment? Also nit: "default". Assuming it's
the insn that's meant, how about "when it's an evex_default one"?

> @@ -9600,10 +9608,21 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>  	  && ins.vex.prefix == DATA_PREFIX_OPCODE)
>  	sizeflag ^= DFLAG;
>  
> +      if(ins.evex_type == evex_default)
> +	ins.vex.nf = false;
> +      else
> +	/* For EVEX-promoted formats, we need to clear EVEX.NF (For ccmp and
> +	   ctest, they will be cleared separately.) in mask_register_specifier
> +	   and keep the low 2 bits of mask_register_specifier to report errors
> +	   for invalid cases.*/
> +	ins.vex.mask_register_specifier &= 0x3;
> +
>        if (dp != NULL && putop (&ins, dp->name, sizeflag) == 0)
>  	{
>  	  if (!get_sib (&ins, sizeflag))
>  	    goto fetch_error_out;
> +	  if (!ins.has_nf && ins.vex.nf)
> +	    oappend (&ins, "{bad}");

As can be seen from the testcase additions, this suffixes onto the insn
menmonic. Since it's a bogus (pseudo) prefix, maybe better also print as
such, and then maybe also indicate what it is that is bad (e.g. {bad-nf})?

> @@ -10564,6 +10583,29 @@ putop (instr_info *ins, const char *in_template, int sizeflag)
>  	    }
>  	  else if (l == 1 && last[0] == 'C')
>  	    break;
> +	  else if (l == 1 && last[0] == 'N')
> +	    {
> +	      ins->has_nf = true;
> +
> +	      if (ins->vex.nf == true)
> +		{
> +		  *ins->obufp++ = '{';
> +		  *ins->obufp++ = 'n';
> +		  *ins->obufp++ = 'f';
> +		  *ins->obufp++ = '}';
> +		  *ins->obufp++ = ' ';
> +		}

Do we really need two booleans here? Can't you simply clear ->vex.nf here,
making print_insn() spot when it's still set?

> +	      else if (ins->evex_type == evex_from_legacy && !ins->vex.b)
> +		{
> +		  *ins->obufp++ = '{';
> +		  *ins->obufp++ = 'e';
> +		  *ins->obufp++ = 'v';
> +		  *ins->obufp++ = 'e';
> +		  *ins->obufp++ = 'x';
> +		  *ins->obufp++ = '}';
> +		  *ins->obufp++ = ' ';
> +		}

This looks unrelated (by the title perhaps more related to the other patch
you sent subsequently), and is also not tested anywhere if I'm not mistaken.
I don't mind it being kept here, but besides needing at least minimal
testing this is then also something that wants mentioning in the
description (for going beyond the basic purpose of the patch). Plus this
effect also needs mentioning above in the documentation of "NF".

Finally for both parts: Why not oappend() or even oappend_with_style()? I
realize we have other similar pieces of code, but I question their
correctness the latest after styling was added.

> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -310,32 +310,42 @@ sti, 0xfb, 0, NoSuf, {}
>  // Arithmetic.
>  add, 0x0, APX_F, D|C|W|CheckOperandSize|Modrm|No_sSuf|DstVVVV|EVexMap4|NF, { Reg8|Reg16|Reg32|Reg64, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg8|Reg16|Reg32|Reg64 }
>  add, 0x0, 0, D|W|CheckOperandSize|Modrm|No_sSuf|HLEPrefixLock, { Reg8|Reg16|Reg32|Reg64, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> +add, 0x0, APX_F, D|W|CheckOperandSize|Modrm|No_sSuf|EVexMap4|NF, { Reg8|Reg16|Reg32|Reg64, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
>  add, 0x83/0, APX_F, Modrm|CheckOperandSize|No_bSuf|No_sSuf|DstVVVV|EVexMap4|NF, { Imm8S, Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }
>  add, 0x83/0, 0, Modrm|No_bSuf|No_sSuf|HLEPrefixLock, { Imm8S, Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> +add, 0x83/0, APX_F, Modrm|No_bSuf|No_sSuf|EVexMap4|NF, { Imm8S, Reg16|Reg32|Reg64|Unspecified|BaseIndex }
>  add, 0x4, 0, W|No_sSuf, { Imm8|Imm16|Imm32|Imm32S, Acc|Byte|Word|Dword|Qword }
>  add, 0x80/0, APX_F, W|Modrm|CheckOperandSize|No_sSuf|DstVVVV|EVexMap4|NF, { Imm8|Imm16|Imm32|Imm32S, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg8|Reg16|Reg32|Reg64}
>  add, 0x80/0, 0, W|Modrm|No_sSuf|HLEPrefixLock, { Imm8|Imm16|Imm32|Imm32S, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> +add, 0x80/0, APX_F, W|Modrm|No_sSuf|EVexMap4|NF, { Imm8|Imm16|Imm32|Imm32S, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }

As before, this (and similar new ones) can also be picked up without use of
{nf}, which will want testing. If you deliberately defer adding such a test,
this needs saying in the description.

One further remark: In the course of adding APX support the set of templates
for ADD and its sibling ALU ops was bumped from 4 to 10. There was quite a
bit of redundancy between these 7 groups before, but it's growing much worse
now. Did you consider templatizing them up front? This may even be worthwhile
for just the ...

>  inc, 0x40, No64, No_bSuf|No_sSuf|No_qSuf, { Reg16|Reg32 }
>  inc, 0xfe/0, APX_F, W|Modrm|No_sSuf|CheckOperandSize|DstVVVV|EVexMap4|NF, {Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg8|Reg16|Reg32|Reg64}
>  inc, 0xfe/0, 0, W|Modrm|No_sSuf|HLEPrefixLock, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> +inc, 0xfe/0, APX_F, W|Modrm|No_sSuf|EVexMap4|NF, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }

... pair of INC/DEC as well, despite it their number only going from 2 to 4.
And quite certainly also for others below.

> @@ -434,24 +464,34 @@ imul, 0x69, i186, Modrm|No_bSuf|No_sSuf|RegKludge, { Imm16|Imm32|Imm32S, Reg16|R
>  
>  div, 0xf6/6, 0, W|Modrm|No_sSuf, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
>  div, 0xf6/6, 0, W|CheckOperandSize|Modrm|No_sSuf, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex, Acc|Byte|Word|Dword|Qword }
> +div, 0xf6/6, APX_F, W|Modrm|No_sSuf|EVexMap4|NF, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
>  idiv, 0xf6/7, 0, W|Modrm|No_sSuf, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
>  idiv, 0xf6/7, 0, W|CheckOperandSize|Modrm|No_sSuf, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex, Acc|Byte|Word|Dword|Qword }
> +idiv, 0xf6/7, APX_F, W|Modrm|No_sSuf|EVexMap4|NF, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }

What about the secondary forms with an explicit destination operand?

> @@ -2027,6 +2090,7 @@ blsi, 0xf3/3, APX_F(BMI), Modrm|CheckOperandSize|Vex128|EVex128|Space0F38|VexVVV
>  blsmsk, 0xf3/2, APX_F(BMI), Modrm|CheckOperandSize|Vex128|EVex128|Space0F38|VexVVVV|No_bSuf|No_wSuf|No_sSuf|NF, { Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64 }
>  blsr, 0xf3/1, APX_F(BMI), Modrm|CheckOperandSize|Vex128|EVex128|Space0F38|VexVVVV|No_bSuf|No_wSuf|No_sSuf|NF, { Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64 }
>  tzcnt, 0xf30fbc, BMI, Modrm|CheckOperandSize|No_bSuf|No_sSuf, { Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }
> +tzcnt, 0xf4, BMI&APX_F, Modrm|CheckOperandSize|No_bSuf|No_sSuf|EVexMap4|NF, { Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }

Like e.g. here, ...

> @@ -2104,9 +2168,11 @@ insertq, 0xf20f78, SSE4a, Modrm|NoSuf, { Imm8, Imm8, RegXMM, RegXMM }
>  
>  // LZCNT instruction
>  lzcnt, 0xf30fbd, LZCNT, Modrm|CheckOperandSize|No_bSuf|No_sSuf, { Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }
> +lzcnt, 0xf5, LZCNT|APX_F, Modrm|CheckOperandSize|No_bSuf|No_sSuf|EVexMap4|NF, { Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }

... I think you mean LZCNT&APX_F here.

>  // POPCNT instruction
>  popcnt, 0xf30fb8, POPCNT, Modrm|CheckOperandSize|No_bSuf|No_sSuf, { Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }
> +popcnt, 0x88, POPCNT|APX_F, Modrm|CheckOperandSize|No_bSuf|No_sSuf|EVexMap4|NF, { Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }

Whereas here it ought to be just APX_F if the doc is to be trusted.

Jan