[v3,4/9] Support APX GPR32 with extend evex prefix

  This patch adds non-ND, non-NF forms of EVEX promotion insn.

EVEX extension of legacy instructions:
  All promoted legacy instructions are placed in EVEX map 4, which is
  currently reserved.
EVEX extension of EVEX instructions:
  All existing EVEX instructions are extended by APX using the extended
  EVEX prefix, so that they can access all 32 GPRs.
EVEX extension of VEX instructions:
  Promoting a VEX instruction into the EVEX space does not change the map
  id, the opcode, or the operand encoding of the VEX instruction.

Note: The promoted versions of MOVBE will be extended to include the “MOVBE
  reg1, reg2”.

  gas/ChangeLog:

  2023-11-21  Lingling Kong <lingling.kong@intel.com>
	      H.J. Lu  <hongjiu.lu@intel.com>
	      Lili Cui <lili.cui@intel.com>
	      Lin Hu   <lin1.hu@intel.com>

	* config/tc-i386.c (cpu_flags_not_or_check): Add a new
	function for APX cpu flag checking.
	(cpu_flags_match): handle cpu_flags_not_or_check.
	(install_template): Add AMX_TILE and APX combine.
	(is_any_apx_evex_encoding): Test apx evex encoding.
	(build_apx_evex_prefix): Enabe APX evex prefix.
	(md_assemble): Handle apx with evex encoding.
	(check_EgprOperands): Add nodgpr check for apx.
	(process_suffix): Handle apx map4 prefix.
	(check_register): Assign i.vec_encoding for APX evex instructions.
	* testsuite/gas/i386/x86-64-evex.d: Adjust test cases.
	* gas/testsuite/gas/i386/x86-64-inval-movbe.s: Ditto.
	* gas/testsuite/gas/i386/x86-64-inval-movbe.l: Ditto.

opcodes/ChangeLog:

	* i386-dis-evex-len.h: Handle EVEX_LEN_0F38F2, EVEX_LEN_0F38F3.
	* i386-dis-evex-prefix.h: Handle PREFIX_EVEX_0F38F2_L_0,
	PREFIX_EVEX_0F38F3_L_0, PREFIX_EVEX_MAP4_D8,
	PREFIX_EVEX_MAP4_DA, PREFIX_EVEX_MAP4_D8,
	PREFIX_EVEX_MAP4_DA, PREFIX_EVEX_MAP4_DB,
	PREFIX_EVEX_MAP4_DC, PREFIX_EVEX_MAP4_DD,
	PREFIX_EVEX_MAP4_DE, PREFIX_EVEX_MAP4_DF,
	PREFIX_EVEX_MAP4_F0, PREFIX_EVEX_MAP4_F1,
	PREFIX_EVEX_MAP4_F2, PREFIX_EVEX_MAP4_F8.
	* i386-dis-evex-reg.h: Handle REG_EVEX_0F38F3_L_0_P_0.
	* i386-dis-evex.h: Add EVEX_MAP4_ for legacy insn
	promote to apx to use gpr32
	* opcodes/i386-dis-evex-x86-64.h: Handle Add X86_64_EVEX_0F90,
	X86_64_EVEX_0F92, X86_64_EVEX_0F93, X86_64_EVEX_0F3849,
	X86_64_EVEX_0F384B, X86_64_EVEX_0F38E0, X86_64_EVEX_0F38E1,
	X86_64_EVEX_0F38E2, X86_64_EVEX_0F38E3, X86_64_EVEX_0F38E4,
	X86_64_EVEX_0F38E5, X86_64_EVEX_0F38E6, X86_64_EVEX_0F38E7,
	X86_64_EVEX_0F38E8, X86_64_EVEX_0F38E9, X86_64_EVEX_0F38EA,
	X86_64_EVEX_0F38EB, X86_64_EVEX_0F38EC, X86_64_EVEX_0F38ED,
	X86_64_EVEX_0F38EE, X86_64_EVEX_0F38EF, X86_64_EVEX_0F38F2,
	X86_64_EVEX_0F38F3, X86_64_EVEX_0F38F5, X86_64_EVEX_0F38F6,
	X86_64_EVEX_0F38F7, X86_64_EVEX_0F3AF0, X86_64_EVEX_0F91.
	* i386-dis.c
	(struct instr_info): Deleted bool r.
	(PREFIX_NP_OR_DATA): New.
	(NO_PREFIX): New.
	(putop): Ditto.
	(X86_64_EVEX_FROM_VEX_TABLE): Diito.
	(get_valid_dis386): Decode insn erex in extend evex prefix.
	Handle EVEX_MAP4
	(print_insn): Handle PREFIX_DATA_AND_NP_ONLY.
	(print_register): Handle apx instructions decode.
	(OP_E_memory): Diito.
	(OP_G): Diito.
	(OP_XMM): Diito.
	(DistinctDest_Fixup): Diito.
	* i386-gen.c (process_i386_opcode_modifier):
	* i386-opc.h (SPACE_EVEXMAP4): Add legacy insn
	promote to evex.
	* i386-opc.tbl: Handle some legacy and vex insns don't
	support gpr32. And add some legacy insn (map2 / 3) promote
	to evex.
---
 gas/config/tc-i386.c                 |  81 ++++++++++++---
 gas/testsuite/gas/i386/x86-64-evex.d |   2 +-
 gas/testsuite/gas/i386/x86-64.exp    |   2 +-
 opcodes/i386-dis-evex-len.h          |  10 ++
 opcodes/i386-dis-evex-prefix.h       |  66 +++++++++++++
 opcodes/i386-dis-evex-reg.h          |   7 ++
 opcodes/i386-dis-evex-x86-64.h       |  60 +++++++++++
 opcodes/i386-dis-evex.h              |  94 +++++++++---------
 opcodes/i386-dis.c                   | 142 +++++++++++++++++++++++----
 opcodes/i386-gen.c                   |   2 +
 opcodes/i386-opc.h                   |  10 ++
 opcodes/i386-opc.tbl                 |  90 +++++++++++------
 12 files changed, 455 insertions(+), 111 deletions(-)
 create mode 100644 opcodes/i386-dis-evex-x86-64.h
  

On 24.11.2023 08:02, Cui, Lili wrote:
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -409,6 +409,9 @@ struct _i386_insn
>      /* Compressed disp8*N attribute.  */
>      unsigned int memshift;
>  
> +    /* No CSPAZO flags update.*/
> +    bool has_nf;

As before I don't see the point in adding this field when it's not used
in the change. Note that this is unrelated to the introduction of the NF
attribute right here, which has a reason.

> @@ -3670,10 +3673,11 @@ install_template (const insn_template *t)
>  
>    /* Dual VEX/EVEX templates need stripping one of the possible variants.  */
>    if (t->opcode_modifier.vex && t->opcode_modifier.evex)
> -  {
> -      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
> -	   || maybe_cpu (t, CpuFMA))
> -	  && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL)))
> +    {
> +      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
> +	  || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) || APX_F(CpuCMPCCXADD)
> +	  || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) || APX_F(CpuAVX512DQ)
> +	  || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
>  	{
>  	  if (need_evex_encoding ())

There are several issues here:
- Why did you need to change (to the worse) the original code?
- Why did you not model the addition after that original code?
- How come APX_F (CpuAVX512*) constructs appear here, when no AVX512 insn
  can be VEX-encoded?
- If these new macros are really needed for whatever reason, they shouldn't
  be added to opcodes/i386-opc.h when they're useful only in the assembler.
- Style requires a blank before the opening parenthesis in function
  invocations (which also covers function-like macro invocations).

I think I asked before: How is it that you get away without altering
cpu_flags_match(), containing related and quite similar logic?

> @@ -3873,6 +3877,14 @@ is_any_vex_encoding (const insn_template *t)
>    return t->opcode_modifier.vex || t->opcode_modifier.evex;
>  }
>  
> +static INLINE bool
> +is_apx_evex_encoding (void)
> +{
> +  return i.rex2 || i.tm.opcode_space == SPACE_EVEXMAP4
> +    || (i.vex.register_specifier
> +	&& i.vex.register_specifier->reg_flags & RegRex2);
> +}

If you want this to be a function despite being used just once, you'll
need to add a comment mentioning the constraint when calling it (or
else the use of i.rex2 in particular is confusing). I'm sure I commented
on this before, and I thought such a comment had already appeared.

> @@ -5655,17 +5693,17 @@ md_assemble (char *line)
>       instruction already has a prefix, we need to convert old
>       registers to new ones.  */
>  
> -  if ((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte
> -       && (i.op[0].regs->reg_flags & RegRex64) != 0)
> -      || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte
> -	  && (i.op[1].regs->reg_flags & RegRex64) != 0)
> -      || (((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte)
> -	   || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte))
> -	  && (i.rex != 0 || i.rex2 != 0)))
> +  if (((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte
> +	&& (i.op[0].regs->reg_flags & RegRex64) != 0)
> +       || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte
> +	   && (i.op[1].regs->reg_flags & RegRex64) != 0)
> +       || (((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte)
> +	    || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte))
> +	   && (i.rex != 0 || i.rex2 != 0))))

I'm having trouble spotting the change here: There's an outer pair of
parentheses being added, but that's for no reason unless there's another
change well hidden. Please clarify.

>      {
>        int x;
>  
> -      if (!i.rex2)
> +      if (!is_apx_rex2_encoding () && !is_any_vex_encoding(&i.tm))
>  	i.rex |= REX_OPCODE;

Why the change to is_apx_rex2_encoding()? If that's wanted / needed
here, shouldn't that be put in place by the earlier patch?

> @@ -14233,6 +14276,12 @@ static bool check_register (const reg_entry *r)
>        if (!cpu_arch_flags.bitfield.cpuapx_f
>  	  || flag_code != CODE_64BIT)
>  	return false;
> +
> +      /* When using RegRex2, dual VEX/EVEX templates need to be marked as EVEX.
> +	 For the later install_template function.  */
> +      if (current_templates->start->opcode_modifier.vex
> +	  && current_templates->start->opcode_modifier.evex)
> +	i.vec_encoding = vex_encoding_evex;

I'm afraid I don't understand the 2nd sentence of the comment. This may
be related to my question regarding cpu_flags_match() further up.

The first sentence isn't quite correct either - you don't mark any
template here (and you can't, because we don't even know yet which
template we're going to use).

Finally - do you really need the .evex check here? (I won't exclude
that this yields a better diagnostic in certain cases, but this wants
clarifying if so.)

> --- a/gas/testsuite/gas/i386/x86-64.exp
> +++ b/gas/testsuite/gas/i386/x86-64.exp
> @@ -250,7 +250,7 @@ run_dump_test "x86-64-sse-noavx"
>  run_dump_test "x86-64-movbe"
>  run_dump_test "x86-64-movbe-intel"
>  run_dump_test "x86-64-movbe-suffix"
> -run_list_test "x86-64-inval-movbe" "-al"
> +run_list_test "x86-64-inval-movbe" "-I${srcdir}/$subdir -march=+noapx_f -al"

I can see why you add the -march=, as we've been through this before.
But why the -I ?

> @@ -896,7 +897,7 @@ rex.wrxb, 0x4f, x64, NoSuf|IsPrefix, {}
>  <pseudopfx:ident:cpu, disp8:Disp8:0, disp16:Disp16:0, disp32:Disp32:0, +
>                        load:Load:0, store:Store:0, +
>                        vex:VEX:0, vex2:VEX:0, vex3:VEX3:0, evex:EVEX:0, +
> -                      rex:REX:x64, rex2:REX2:x64, nooptimize:NoOptimize:0>
> +                      rex:REX:x64, rex2:REX2:APX_F, nooptimize:NoOptimize:0>

This change wants to go into the earlier patch?

> @@ -1319,13 +1320,16 @@ getsec, 0xf37, SMX, NoSuf, {}
>  
>  invept, 0x660f3880, EPT&No64, Modrm|IgnoreSize|NoSuf, { Oword|Unspecified|BaseIndex, Reg32 }
>  invept, 0x660f3880, EPT&x64, Modrm|NoSuf|NoRex64, { Oword|Unspecified|BaseIndex, Reg64 }
> +invept, 0xf3f0, EPT&APX_F, Modrm|NoSuf|EVex128|EVexMap4, { Oword|Unspecified|BaseIndex, Reg64 }
>  invvpid, 0x660f3881, EPT&No64, Modrm|IgnoreSize|NoSuf, { Oword|Unspecified|BaseIndex, Reg32 }
>  invvpid, 0x660f3881, EPT&x64, Modrm|NoSuf|NoRex64, { Oword|Unspecified|BaseIndex, Reg64 }
> +invvpid, 0xf3f1, EPT&APX_F, Modrm|NoSuf|EVex128|EVexMap4, { Oword|Unspecified|BaseIndex, Reg64 }

Seeing these: Are there any Map4 encodings which aren't EVex128? If not (and
if you're also not hiddenly aware of some appearing in the near future),
please consider making EVexMap4 include this right away. Even if in the
longer run other encodings appear, it'll then be easy to simply replace all
the EVexMap4 uses in a purely mechanical way. Until then shorter template
lines are preferable.

> @@ -1437,7 +1443,6 @@ xgetbv, 0xf01d0, Xsave, NoSuf, {}
>  xsetbv, 0xf01d1, Xsave, NoSuf, {}
>  
>  // xsaveopt
> -
>  xsaveopt, 0xfae/6, Xsaveopt, Modrm|No_bSuf|No_wSuf|No_lSuf|No_sSuf|NoEgpr, { Unspecified|BaseIndex }
>  xsaveopt64, 0xfae/6, Xsaveopt&x64, Modrm|NoSuf|Size64|NoEgpr, { Unspecified|BaseIndex }

Iirc the earlier patch added that blank line. Why would you do such back
and forth?

> @@ -1837,14 +1842,14 @@ xtest, 0xf01d6, HLE|RTM, NoSuf, {}
>  
>  // BMI2 instructions.
>  
> -bzhi, 0xf5, BMI2, Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|SwapSources|No_bSuf|No_wSuf|No_sSuf, { Reg32|Reg64, Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64 }
> -mulx, 0xf2f6, BMI2, Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|No_bSuf|No_wSuf|No_sSuf, { Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64, Reg32|Reg64 }
> -pdep, 0xf2f5, BMI2, Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|No_bSuf|No_wSuf|No_sSuf, { Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64, Reg32|Reg64 }
> -pext, 0xf3f5, BMI2, Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|No_bSuf|No_wSuf|No_sSuf, { Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64, Reg32|Reg64 }
> -rorx, 0xf2f0, BMI2, Modrm|CheckOperandSize|Vex128|Space0F3A|No_bSuf|No_wSuf|No_sSuf, { Imm8|Imm8S, Reg32|Reg64|Dword|Qword|Unspecified|BaseIndex, Reg32|Reg64 }
> -sarx, 0xf3f7, BMI2, Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|SwapSources|No_bSuf|No_wSuf|No_sSuf, { Reg32|Reg64, Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64 }
> -shlx, 0x66f7, BMI2, Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|SwapSources|No_bSuf|No_wSuf|No_sSuf, { Reg32|Reg64, Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64 }
> -shrx, 0xf2f7, BMI2, Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|SwapSources|No_bSuf|No_wSuf|No_sSuf, { Reg32|Reg64, Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64 }
> +bzhi, 0xf5, BMI2&(BMI2|APX_F), Modrm|CheckOperandSize|Vex128|EVex128|Space0F38|VexVVVV|SwapSources|No_bSuf|No_wSuf|No_sSuf|NF, { Reg32|Reg64, Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64 }

Hmm, I had specifically suggested a pre-processor macro to use in place of the
open-coded BMI2&(BMI2|APX_F). Is there a reason you didn't use that (here and
below)?

Jan
  

On 24.11.2023 08:02, Cui, Lili wrote:
> --- /dev/null
> +++ b/opcodes/i386-dis-evex-x86-64.h
> @@ -0,0 +1,60 @@
> +  /* X86_64_EVEX_0F90 */
> +  {
> +    { Bad_Opcode },
> +    { VEX_LEN_TABLE (VEX_LEN_0F90) },
> +  },
> +  /* X86_64_EVEX_0F91 */
> +  {
> +    { Bad_Opcode },
> +    { VEX_LEN_TABLE (VEX_LEN_0F91) },
> +  },
> +  /* X86_64_EVEX_0F92 */
> +  {
> +    { Bad_Opcode },
> +    { VEX_LEN_TABLE (VEX_LEN_0F92) },
> +  },
> +  /* X86_64_EVEX_0F93 */
> +  {
> +    { Bad_Opcode },
> +    { VEX_LEN_TABLE (VEX_LEN_0F93) },
> +  },
> +  /* X86_64_EVEX_0F3849 */
> +  {
> +    { Bad_Opcode },
> +    { VEX_LEN_TABLE (VEX_LEN_0F3849_X86_64) },
> +  },
> +  /* X86_64_EVEX_0F384B */
> +  {
> +    { Bad_Opcode },
> +    { VEX_LEN_TABLE (VEX_LEN_0F384B_X86_64) },
> +  },
> +  /* X86_64_EVEX_0F38F2 */
> +  {
> +    { Bad_Opcode },
> +    { EVEX_LEN_TABLE (EVEX_LEN_0F38F2) },
> +  },
> +  /* X86_64_EVEX_0F38F3 */
> +  {
> +    { Bad_Opcode },
> +    { EVEX_LEN_TABLE (EVEX_LEN_0F38F3) },
> +  },
> +  /* X86_64_EVEX_0F38F5 */
> +  {
> +    { Bad_Opcode },
> +    { VEX_LEN_TABLE (VEX_LEN_0F38F5) },
> +  },
> +  /* X86_64_EVEX_0F38F6 */
> +  {
> +    { Bad_Opcode },
> +    { VEX_LEN_TABLE (VEX_LEN_0F38F6) },
> +  },
> +  /* X86_64_EVEX_0F38F7 */
> +  {
> +    { Bad_Opcode },
> +    { VEX_LEN_TABLE (VEX_LEN_0F38F7) },
> +  },
> +  /* X86_64_EVEX_0F3AF0 */
> +  {
> +    { Bad_Opcode },
> +    { VEX_LEN_TABLE (VEX_LEN_0F3AF0) },
> +  },

I'm puzzled here: There are two uses of EVEX_LEN_TABLE() and several more
of VEX_LEN_TABLE(). Yet the underlying pattern of those insns is all the
same. I may guess that this is related to PREFIX_OPCODE use in the
respective VEX table entries, yet isn't it then cheaper overall to have
VEX encodings also go through prefix_table[], and then sharing those
entries with EVEX encodings?

What's further puzzling: When setting evex_from_vex you already check
L'L == 0, so there's no reason to go through evex_len_table[] /
vex_len_table[].

> @@ -1268,7 +1296,21 @@ enum
>    X86_64_VEX_0F38ED,
>    X86_64_VEX_0F38EE,
>    X86_64_VEX_0F38EF,
> +
>    X86_64_VEX_MAP7_F8_L_0_W_0_R_0,
> +
> +  X86_64_EVEX_0F90,
> +  X86_64_EVEX_0F91,
> +  X86_64_EVEX_0F92,
> +  X86_64_EVEX_0F93,
> +  X86_64_EVEX_0F3849,
> +  X86_64_EVEX_0F384B,

For these two, won't the respective VEX enumerators and table entries
do?

> @@ -4524,10 +4568,11 @@ static const struct dis386 x86_64_table[][2] = {
>  
>    /* X86_64_VEX_MAP7_F8_L_0_W_0_R_0 */
>    {
> -    { Bad_Opcode },
> -    { PREFIX_TABLE (PREFIX_VEX_MAP7_F8_L_0_W_0_R_0_X86_64) },
> +      { Bad_Opcode },
> +      { PREFIX_TABLE (PREFIX_VEX_MAP7_F8_L_0_W_0_R_0_X86_64) },
>    },

Actively corrupting indentation here?

> @@ -8733,6 +8778,17 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
>        dp = &prefix_table[dp->op[1].bytemode][vindex];
>        break;
>  
> +    case USE_X86_64_EVEX_FROM_VEX_TABLE:
> +      ins->evex_type = evex_from_vex;
> +      /* EVEX from evex instrucions require that EVEX.z, EVEX.L’L, EVEX.b and

"EVEX from VEX ..."?

> +	 the lower 2 bits of EVEX.aaa must be 0.  */
> +      if ((ins->vex.mask_register_specifier & 0x3) != 0
> +	  || ins->vex.ll != 0
> +	  || ins->vex.zeroing != 0
> +	  || ins->vex.b)
> +	return &bad_opcode;
> +
> +      /* Fall through.  */
>      case USE_X86_64_TABLE:

Instead of falling through here to go through x86_64_table[] (where in all
cases the non-64-bit slot is "bad"), can't you avoid that step and go to
the next step (uniformly the LEN one) right away, saving all those new
table entries (along the lines of what you do below when processing into
evex_from_legacy)?

> @@ -8978,9 +9034,13 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
>        if (!fetch_code (ins->info, ins->codep + 4))
>  	return &err_opcode;
>        /* The first byte after 0x62.  */
> +      if (*ins->codep & 0x8)
> +	ins->rex2 |= REX_B;
> +      if (!(*ins->codep & 0x10))
> +	ins->rex2 |= REX_R;
> +
>        ins->rex = ~(*ins->codep >> 5) & 0x7;
> -      ins->vex.r = *ins->codep & 0x10;
> -      switch ((*ins->codep & 0xf))
> +      switch ((*ins->codep & 0x7))

Please can you take the opportunity and drop the excess parentheses?

> @@ -9041,12 +9106,24 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
>  
>        if (ins->address_mode != mode_64bit)
>  	{
> +	  if (ins->evex_type != evex_default
> +	      || (ins->rex2 & (REX_B | REX_X)))
> +	    return &bad_opcode;

What's special about X and B?

> @@ -9460,6 +9537,13 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>        dp = get_valid_dis386 (dp, &ins);
>        if (dp == &err_opcode)
>  	goto fetch_error_out;
> +
> +      /* For APX instructions promoted from legacy maps 0/1, prefix
> +	 0x66 is interpreted as the operand size override.  */
> +      if (ins.evex_type == evex_from_legacy
> +	  && ins.vex.prefix == DATA_PREFIX_OPCODE)
> +	sizeflag ^= DFLAG;

I think the comment wants to say "embedded prefix", as "prefix 0x66" is
simply invalid to use with EVEX.

> @@ -9639,6 +9723,24 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>        if (ins.last_repnz_prefix >= 0)
>  	ins.all_prefixes[ins.last_repnz_prefix] = 0xf2;
>        break;
> +
> +    case PREFIX_NP_OR_DATA:
> +      if (ins.vex.prefix & ~DATA_PREFIX_OPCODE)

~DATA_PREFIX_OPCODE == 0x99, which likely isn't what you mean here? Do
you perhaps mean e.g. "> DATA_PREFIX_OPCODE"? (Using the opcodes in
vex.prefix is questionable anyway, but that's a pre-existing oddity.)

Jan
  

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, December 7, 2023 8:39 PM
> To: Cui, Lili <lili.cui@intel.com>
> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; binutils@sourceware.org
> Subject: Re: [PATCH v3 4/9] Support APX GPR32 with extend evex prefix
> 
> On 24.11.2023 08:02, Cui, Lili wrote:
> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -409,6 +409,9 @@ struct _i386_insn
> >      /* Compressed disp8*N attribute.  */
> >      unsigned int memshift;
> >
> > +    /* No CSPAZO flags update.*/
> > +    bool has_nf;
> 
> As before I don't see the point in adding this field when it's not used in the
> change. Note that this is unrelated to the introduction of the NF attribute right
> here, which has a reason.
> 

Moved.

> > @@ -3670,10 +3673,11 @@ install_template (const insn_template *t)
> >
> >    /* Dual VEX/EVEX templates need stripping one of the possible variants.  */
> >    if (t->opcode_modifier.vex && t->opcode_modifier.evex)
> > -  {
> > -      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
> > -	   || maybe_cpu (t, CpuFMA))
> > -	  && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL)))
> > +    {
> > +      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
> > +	  || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
> APX_F(CpuCMPCCXADD)
> > +	  || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
> APX_F(CpuAVX512DQ)
> > +	  || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
> >  	{
> >  	  if (need_evex_encoding ())
> 
> There are several issues here:
> - Why did you need to change (to the worse) the original code?
> - Why did you not model the addition after that original code?
> - How come APX_F (CpuAVX512*) constructs appear here, when no AVX512
> insn can be VEX-encoded?

 I don't understand what you mean, we have this combination.

kmov<dq>, 0x<dq:kpfx>90, AVX512BW&(AVX512BW|APX_F), Modrm|Vex128|EVex128|Space0F|VexW1|<dq:kvsz>|NoSuf, { RegMask|<dq:elem>|Unspecified|BaseIndex, RegMask }

> - If these new macros are really needed for whatever reason, they shouldn't
>   be added to opcodes/i386-opc.h when they're useful only in the assembler.
> - Style requires a blank before the opening parenthesis in function
>   invocations (which also covers function-like macro invocations).
> 
> I think I asked before: How is it that you get away without altering
> cpu_flags_match(), containing related and quite similar logic?
> 

For the original logic ( ... || ... ) && ( ... || ...), the content in the first bracket and the content in the following brackets can be combined arbitrarily. I think it is Inaccurate. So I give examples one by one for each identified combination.

Just found cpu_flags_match() has similar logic, I think the following is the only code related to CPUID alerts, but none of our combinations are related to cpuavx.

          if (all.bitfield.cpuavx)
            {
              /* We need to check SSE2AVX with AVX.  */
              if (!t->opcode_modifier.sse2avx
                  || (sse2avx && !i.prefix[DATA_PREFIX]))
                match |= CPU_FLAGS_ARCH_MATCH;
            }

> > @@ -3873,6 +3877,14 @@ is_any_vex_encoding (const insn_template *t)
> >    return t->opcode_modifier.vex || t->opcode_modifier.evex;  }
> >
> > +static INLINE bool
> > +is_apx_evex_encoding (void)
> > +{
> > +  return i.rex2 || i.tm.opcode_space == SPACE_EVEXMAP4
> > +    || (i.vex.register_specifier
> > +	&& i.vex.register_specifier->reg_flags & RegRex2); }
> 
> If you want this to be a function despite being used just once, you'll need to
> add a comment mentioning the constraint when calling it (or else the use of
> i.rex2 in particular is confusing). I'm sure I commented on this before, and I
> thought such a comment had already appeared.
> 

I also have the impression that it was added, anyway I will add it.

+/* We can use this function only when the current encoding is evex.  */
 static INLINE bool
 is_apx_evex_encoding (void)
 {

> > @@ -5655,17 +5693,17 @@ md_assemble (char *line)
> >       instruction already has a prefix, we need to convert old
> >       registers to new ones.  */
> >
> > -  if ((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte
> > -       && (i.op[0].regs->reg_flags & RegRex64) != 0)
> > -      || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte
> > -	  && (i.op[1].regs->reg_flags & RegRex64) != 0)
> > -      || (((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte)
> > -	   || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte))
> > -	  && (i.rex != 0 || i.rex2 != 0)))
> > +  if (((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte
> > +	&& (i.op[0].regs->reg_flags & RegRex64) != 0)
> > +       || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte
> > +	   && (i.op[1].regs->reg_flags & RegRex64) != 0)
> > +       || (((i.types[0].bitfield.class == Reg && i.types[0].bitfield.byte)
> > +	    || (i.types[1].bitfield.class == Reg && i.types[1].bitfield.byte))
> > +	   && (i.rex != 0 || i.rex2 != 0))))
> 
> I'm having trouble spotting the change here: There's an outer pair of
> parentheses being added, but that's for no reason unless there's another
> change well hidden. Please clarify.
> 

Removed.

> >      {
> >        int x;
> >
> > -      if (!i.rex2)
> > +      if (!is_apx_rex2_encoding () && !is_any_vex_encoding(&i.tm))
> >  	i.rex |= REX_OPCODE;
> 
> Why the change to is_apx_rex2_encoding()? If that's wanted / needed here,
> shouldn't that be put in place by the earlier patch?
>

Moved to the corresponding patch.

> > @@ -14233,6 +14276,12 @@ static bool check_register (const reg_entry
> *r)
> >        if (!cpu_arch_flags.bitfield.cpuapx_f
> >  	  || flag_code != CODE_64BIT)
> >  	return false;
> > +
> > +      /* When using RegRex2, dual VEX/EVEX templates need to be marked as
> EVEX.
> > +	 For the later install_template function.  */
> > +      if (current_templates->start->opcode_modifier.vex
> > +	  && current_templates->start->opcode_modifier.evex)
> > +	i.vec_encoding = vex_encoding_evex;
> 
> I'm afraid I don't understand the 2nd sentence of the comment. This may be
> related to my question regarding cpu_flags_match() further up.
> 
> The first sentence isn't quite correct either - you don't mark any template here
> (and you can't, because we don't even know yet which template we're going
> to use).
> 
> Finally - do you really need the .evex check here? (I won't exclude that this
> yields a better diagnostic in certain cases, but this wants clarifying if so.)
> 

If you look at install_template(), you'll see that before this function we need to know if the current encoding is evex. We need to check opcode_modifier.evex here, it is a fix for issues caused by the merge of VEX and EVEX.
  if (t->opcode_modifier.vex && t->opcode_modifier.evex)
    {
      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
          || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) || APX_F(CpuCMPCCXADD)
          || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) || APX_F(CpuAVX512DQ)
          || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
        {
          if (need_evex_encoding ())
            {

> > --- a/gas/testsuite/gas/i386/x86-64.exp
> > +++ b/gas/testsuite/gas/i386/x86-64.exp
> > @@ -250,7 +250,7 @@ run_dump_test "x86-64-sse-noavx"
> >  run_dump_test "x86-64-movbe"
> >  run_dump_test "x86-64-movbe-intel"
> >  run_dump_test "x86-64-movbe-suffix"
> > -run_list_test "x86-64-inval-movbe" "-al"
> > +run_list_test "x86-64-inval-movbe" "-I${srcdir}/$subdir -march=+noapx_f -
> al"
> 
> I can see why you add the -march=, as we've been through this before.
> But why the -I ?
> 

Removed, It is redundant.

> > @@ -896,7 +897,7 @@ rex.wrxb, 0x4f, x64, NoSuf|IsPrefix, {}
> > <pseudopfx:ident:cpu, disp8:Disp8:0, disp16:Disp16:0, disp32:Disp32:0, +
> >                        load:Load:0, store:Store:0, +
> >                        vex:VEX:0, vex2:VEX:0, vex3:VEX3:0, evex:EVEX:0, +
> > -                      rex:REX:x64, rex2:REX2:x64, nooptimize:NoOptimize:0>
> > +                      rex:REX:x64, rex2:REX2:APX_F,
> > + nooptimize:NoOptimize:0>
> 
> This change wants to go into the earlier patch?
> 

Done.

> > @@ -1319,13 +1320,16 @@ getsec, 0xf37, SMX, NoSuf, {}
> >
> >  invept, 0x660f3880, EPT&No64, Modrm|IgnoreSize|NoSuf, {
> > Oword|Unspecified|BaseIndex, Reg32 }  invept, 0x660f3880, EPT&x64,
> > Modrm|NoSuf|NoRex64, { Oword|Unspecified|BaseIndex, Reg64 }
> > +invept, 0xf3f0, EPT&APX_F, Modrm|NoSuf|EVex128|EVexMap4, {
> > +Oword|Unspecified|BaseIndex, Reg64 }
> >  invvpid, 0x660f3881, EPT&No64, Modrm|IgnoreSize|NoSuf, {
> > Oword|Unspecified|BaseIndex, Reg32 }  invvpid, 0x660f3881, EPT&x64,
> > Modrm|NoSuf|NoRex64, { Oword|Unspecified|BaseIndex, Reg64 }
> > +invvpid, 0xf3f1, EPT&APX_F, Modrm|NoSuf|EVex128|EVexMap4, {
> > +Oword|Unspecified|BaseIndex, Reg64 }
> 
> Seeing these: Are there any Map4 encodings which aren't EVex128? If not
> (and if you're also not hiddenly aware of some appearing in the near future),
> please consider making EVexMap4 include this right away. Even if in the longer
> run other encodings appear, it'll then be easy to simply replace all the
> EVexMap4 uses in a purely mechanical way. Until then shorter template lines
> are preferable.
> 

Would you mind defining it this way? Since #define EVex128 is behind it. Considering that you don't like unnecessary changes.

+#define EVexMap4 OpcodeSpace=SPACE_EVEXMAP4|EVex=EVEX128

> > @@ -1437,7 +1443,6 @@ xgetbv, 0xf01d0, Xsave, NoSuf, {}  xsetbv,
> > 0xf01d1, Xsave, NoSuf, {}
> >
> >  // xsaveopt
> > -
> >  xsaveopt, 0xfae/6, Xsaveopt,
> > Modrm|No_bSuf|No_wSuf|No_lSuf|No_sSuf|NoEgpr,
> { Unspecified|BaseIndex
> > }  xsaveopt64, 0xfae/6, Xsaveopt&x64, Modrm|NoSuf|Size64|NoEgpr, {
> > Unspecified|BaseIndex }
> 
> Iirc the earlier patch added that blank line. Why would you do such back and
> forth?
> 

Done.

> > @@ -1837,14 +1842,14 @@ xtest, 0xf01d6, HLE|RTM, NoSuf, {}
> >
> >  // BMI2 instructions.
> >
> > -bzhi, 0xf5, BMI2,
> >
> Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|SwapSources|No
> _bSuf|No
> > _wSuf|No_sSuf, { Reg32|Reg64, Reg32|Reg64|Unspecified|BaseIndex,
> > Reg32|Reg64 } -mulx, 0xf2f6, BMI2,
> >
> Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|No_bSuf|No_wS
> uf|No_sSu
> > f, { Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64, Reg32|Reg64 }
> > -pdep, 0xf2f5, BMI2,
> >
> Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|No_bSuf|No_wS
> uf|No_sSu
> > f, { Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64, Reg32|Reg64 }
> > -pext, 0xf3f5, BMI2,
> >
> Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|No_bSuf|No_wS
> uf|No_sSu
> > f, { Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64, Reg32|Reg64 }
> > -rorx, 0xf2f0, BMI2,
> >
> Modrm|CheckOperandSize|Vex128|Space0F3A|No_bSuf|No_wSuf|No_sSu
> f, {
> > Imm8|Imm8S, Reg32|Reg64|Dword|Qword|Unspecified|BaseIndex,
> Reg32|Reg64
> > } -sarx, 0xf3f7, BMI2,
> >
> Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|SwapSources|No
> _bSuf|No
> > _wSuf|No_sSuf, { Reg32|Reg64, Reg32|Reg64|Unspecified|BaseIndex,
> > Reg32|Reg64 } -shlx, 0x66f7, BMI2,
> >
> Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|SwapSources|No
> _bSuf|No
> > _wSuf|No_sSuf, { Reg32|Reg64, Reg32|Reg64|Unspecified|BaseIndex,
> > Reg32|Reg64 } -shrx, 0xf2f7, BMI2,
> >
> Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|SwapSources|No
> _bSuf|No
> > _wSuf|No_sSuf, { Reg32|Reg64, Reg32|Reg64|Unspecified|BaseIndex,
> > Reg32|Reg64 }
> > +bzhi, 0xf5, BMI2&(BMI2|APX_F),
> >
> +Modrm|CheckOperandSize|Vex128|EVex128|Space0F38|VexVVVV|SwapS
> ources|N
> > +o_bSuf|No_wSuf|No_sSuf|NF, { Reg32|Reg64,
> > +Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64 }
> 
> Hmm, I had specifically suggested a pre-processor macro to use in place of the
> open-coded BMI2&(BMI2|APX_F). Is there a reason you didn't use that (here
> and below)?
> 

There are many different types of combinations, and each combination appears relatively few times, so I think adding a #define for each combination feels a bit wasteful.

Thanks,
Lili.
  

> On 24.11.2023 08:02, Cui, Lili wrote:
> > --- /dev/null
> > +++ b/opcodes/i386-dis-evex-x86-64.h
> > @@ -0,0 +1,60 @@
> > +  /* X86_64_EVEX_0F90 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F90) },  },
> > +  /* X86_64_EVEX_0F91 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F91) },  },
> > +  /* X86_64_EVEX_0F92 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F92) },  },
> > +  /* X86_64_EVEX_0F93 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F93) },  },
> > +  /* X86_64_EVEX_0F3849 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F3849_X86_64) },  },
> > +  /* X86_64_EVEX_0F384B */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F384B_X86_64) },  },
> > +  /* X86_64_EVEX_0F38F2 */
> > +  {
> > +    { Bad_Opcode },
> > +    { EVEX_LEN_TABLE (EVEX_LEN_0F38F2) },  },
> > +  /* X86_64_EVEX_0F38F3 */
> > +  {
> > +    { Bad_Opcode },
> > +    { EVEX_LEN_TABLE (EVEX_LEN_0F38F3) },  },
> > +  /* X86_64_EVEX_0F38F5 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F38F5) },  },
> > +  /* X86_64_EVEX_0F38F6 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F38F6) },  },
> > +  /* X86_64_EVEX_0F38F7 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F38F7) },  },
> > +  /* X86_64_EVEX_0F3AF0 */
> > +  {
> > +    { Bad_Opcode },
> > +    { VEX_LEN_TABLE (VEX_LEN_0F3AF0) },  },
> 
> I'm puzzled here: There are two uses of EVEX_LEN_TABLE() and several more
> of VEX_LEN_TABLE(). Yet the underlying pattern of those insns is all the same. I
> may guess that this is related to PREFIX_OPCODE use in the respective VEX
> table entries, yet isn't it then cheaper overall to have VEX encodings also go
> through prefix_table[], and then sharing those entries with EVEX encodings?
> 

Done.

> What's further puzzling: When setting evex_from_vex you already check L'L ==
> 0, so there's no reason to go through evex_len_table[] / vex_len_table[].
> 

Directly use the next level of len_table[].

> > @@ -1268,7 +1296,21 @@ enum
> >    X86_64_VEX_0F38ED,
> >    X86_64_VEX_0F38EE,
> >    X86_64_VEX_0F38EF,
> > +
> >    X86_64_VEX_MAP7_F8_L_0_W_0_R_0,
> > +
> > +  X86_64_EVEX_0F90,
> > +  X86_64_EVEX_0F91,
> > +  X86_64_EVEX_0F92,
> > +  X86_64_EVEX_0F93,
> > +  X86_64_EVEX_0F3849,
> > +  X86_64_EVEX_0F384B,
> 
> For these two, won't the respective VEX enumerators and table entries do?
> 

Done.

> > @@ -4524,10 +4568,11 @@ static const struct dis386 x86_64_table[][2] =
> > {
> >
> >    /* X86_64_VEX_MAP7_F8_L_0_W_0_R_0 */
> >    {
> > -    { Bad_Opcode },
> > -    { PREFIX_TABLE (PREFIX_VEX_MAP7_F8_L_0_W_0_R_0_X86_64) },
> > +      { Bad_Opcode },
> > +      { PREFIX_TABLE (PREFIX_VEX_MAP7_F8_L_0_W_0_R_0_X86_64) },
> >    },
> 
> Actively corrupting indentation here?
> 

Done.

> > @@ -8733,6 +8778,17 @@ get_valid_dis386 (const struct dis386 *dp,
> instr_info *ins)
> >        dp = &prefix_table[dp->op[1].bytemode][vindex];
> >        break;
> >
> > +    case USE_X86_64_EVEX_FROM_VEX_TABLE:
> > +      ins->evex_type = evex_from_vex;
> > +      /* EVEX from evex instrucions require that EVEX.z, EVEX.L’L,
> > + EVEX.b and
> 
> "EVEX from VEX ..."?
> 

Done.

> > +	 the lower 2 bits of EVEX.aaa must be 0.  */
> > +      if ((ins->vex.mask_register_specifier & 0x3) != 0
> > +	  || ins->vex.ll != 0
> > +	  || ins->vex.zeroing != 0
> > +	  || ins->vex.b)
> > +	return &bad_opcode;
> > +
> > +      /* Fall through.  */
> >      case USE_X86_64_TABLE:
> 
> Instead of falling through here to go through x86_64_table[] (where in all
> cases the non-64-bit slot is "bad"), can't you avoid that step and go to the
> next step (uniformly the LEN one) right away, saving all those new table entries
> (along the lines of what you do below when processing into
> evex_from_legacy)?
> 

It's not very clear to me here, do you want to add the vex_len_table to delete all entries in i386-dis-evex-x86-64.h?  but in this way, there are still some instructions that need to go through x86_64_table[], such as X86_64_VEX_0F38E*.

    case USE_X86_64_EVEX_FROM_VEX_TABLE:
      ins->evex_type = evex_from_vex;
      /* EVEX from VEX instrucions require that EVEX.z, EVEX.L’L, EVEX.b and
         the lower 2 bits of EVEX.aaa must be 0.  */
      if ((ins->vex.mask_register_specifier & 0x3) != 0
          || ins->vex.ll != 0
          || ins->vex.zeroing != 0
          || ins->vex.b)
        return &bad_opcode;

     dp = &vex_len_table[dp->op[1].bytemode][0];
break;

> > @@ -8978,9 +9034,13 @@ get_valid_dis386 (const struct dis386 *dp,
> instr_info *ins)
> >        if (!fetch_code (ins->info, ins->codep + 4))
> >  	return &err_opcode;
> >        /* The first byte after 0x62.  */
> > +      if (*ins->codep & 0x8)
> > +	ins->rex2 |= REX_B;
> > +      if (!(*ins->codep & 0x10))
> > +	ins->rex2 |= REX_R;
> > +
> >        ins->rex = ~(*ins->codep >> 5) & 0x7;
> > -      ins->vex.r = *ins->codep & 0x10;
> > -      switch ((*ins->codep & 0xf))
> > +      switch ((*ins->codep & 0x7))
> 
> Please can you take the opportunity and drop the excess parentheses?
> 

Done.

> > @@ -9041,12 +9106,24 @@ get_valid_dis386 (const struct dis386 *dp,
> > instr_info *ins)
> >
> >        if (ins->address_mode != mode_64bit)
> >  	{
> > +	  if (ins->evex_type != evex_default
> > +	      || (ins->rex2 & (REX_B | REX_X)))
> > +	    return &bad_opcode;
> 
> What's special about X and B?
> 

For evex_default, the values of these two bits are fixed. Comment added.

      if (ins->address_mode != mode_64bit)
        {
          /* Report bad for !evex_default and when two fixed values of evex
             change..  */
          if (ins->evex_type != evex_default
              || (ins->rex2 & (REX_B | REX_X)))
            return &bad_opcode;

> > @@ -9460,6 +9537,13 @@ print_insn (bfd_vma pc, disassemble_info *info,
> int intel_syntax)
> >        dp = get_valid_dis386 (dp, &ins);
> >        if (dp == &err_opcode)
> >  	goto fetch_error_out;
> > +
> > +      /* For APX instructions promoted from legacy maps 0/1, prefix
> > +	 0x66 is interpreted as the operand size override.  */
> > +      if (ins.evex_type == evex_from_legacy
> > +	  && ins.vex.prefix == DATA_PREFIX_OPCODE)
> > +	sizeflag ^= DFLAG;
> 
> I think the comment wants to say "embedded prefix", as "prefix 0x66" is
> simply invalid to use with EVEX.
> 

Done, thanks.

> > @@ -9639,6 +9723,24 @@ print_insn (bfd_vma pc, disassemble_info *info,
> int intel_syntax)
> >        if (ins.last_repnz_prefix >= 0)
> >  	ins.all_prefixes[ins.last_repnz_prefix] = 0xf2;
> >        break;
> > +
> > +    case PREFIX_NP_OR_DATA:
> > +      if (ins.vex.prefix & ~DATA_PREFIX_OPCODE)
> 
> ~DATA_PREFIX_OPCODE == 0x99, which likely isn't what you mean here? Do
> you perhaps mean e.g. "> DATA_PREFIX_OPCODE"? (Using the opcodes in
> vex.prefix is questionable anyway, but that's a pre-existing oddity.)
> 

(A || 0) & ~A must be 0. It's hard to read.  

How about this ? This is more intuitive and easy to understand.

    case PREFIX_NP_OR_DATA:
      if (ins.vex.prefix == REPE_PREFIX_OPCODE
          || ins.vex.prefix == REPNE_PREFIX_OPCODE)
        {
          i386_dis_printf (info, dis_style_text, "(bad)");
          ret = ins.end_codep - priv.the_buffer;
          goto out;
        }

Thanks,
Lili.
  

On 08.12.2023 16:21, Cui, Lili wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Thursday, December 7, 2023 8:39 PM
>>
>> On 24.11.2023 08:02, Cui, Lili wrote:
>>> @@ -3670,10 +3673,11 @@ install_template (const insn_template *t)
>>>
>>>    /* Dual VEX/EVEX templates need stripping one of the possible variants.  */
>>>    if (t->opcode_modifier.vex && t->opcode_modifier.evex)
>>> -  {
>>> -      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
>>> -	   || maybe_cpu (t, CpuFMA))
>>> -	  && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL)))
>>> +    {
>>> +      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
>>> +	  || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
>> APX_F(CpuCMPCCXADD)
>>> +	  || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
>> APX_F(CpuAVX512DQ)
>>> +	  || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
>>>  	{
>>>  	  if (need_evex_encoding ())
>>
>> There are several issues here:
>> - Why did you need to change (to the worse) the original code?
>> - Why did you not model the addition after that original code?
>> - How come APX_F (CpuAVX512*) constructs appear here, when no AVX512
>> insn can be VEX-encoded?
> 
>  I don't understand what you mean, we have this combination.
> 
> kmov<dq>, 0x<dq:kpfx>90, AVX512BW&(AVX512BW|APX_F), Modrm|Vex128|EVex128|Space0F|VexW1|<dq:kvsz>|NoSuf, { RegMask|<dq:elem>|Unspecified|BaseIndex, RegMask }

Oh, I'm sorry: I forgot about the mask register insns.

>> - If these new macros are really needed for whatever reason, they shouldn't
>>   be added to opcodes/i386-opc.h when they're useful only in the assembler.
>> - Style requires a blank before the opening parenthesis in function
>>   invocations (which also covers function-like macro invocations).
>>
>> I think I asked before: How is it that you get away without altering
>> cpu_flags_match(), containing related and quite similar logic?
>>
> 
> For the original logic ( ... || ... ) && ( ... || ...), the content in the first bracket and the content in the following brackets can be combined arbitrarily. I think it is Inaccurate.

In which way? If there are issues with the existing code, these issues want
taking care of in separate (prereq) patches. Of course there are assumptions
made here about the CPU combinations that can (and cannot) occur in any of
our templates. Similar assumptions are imo fine to make in the APX additions.

Note how I used two nested if()s despite that not having been necessary at
that time. I did so in anticipation that for APX you'd want to add another
(separate) inner if(), rather than altering the one that's there.

> So I give examples one by one for each identified combination.

Which examples are you talking about? I see none given in your reply.

> Just found cpu_flags_match() has similar logic, I think the following is the only code related to CPUID alerts, but none of our combinations are related to cpuavx.
> 
>           if (all.bitfield.cpuavx)
>             {
>               /* We need to check SSE2AVX with AVX.  */
>               if (!t->opcode_modifier.sse2avx
>                   || (sse2avx && !i.prefix[DATA_PREFIX]))
>                 match |= CPU_FLAGS_ARCH_MATCH;
>             }

Not sure why you pick out this one. This special case is needed for sse2avx;
I don't see how it's related here. What I've been pointing you at is the
code in that function which follows a similar "Dual VEX/EVEX templates ..."
comment.

>>> @@ -14233,6 +14276,12 @@ static bool check_register (const reg_entry
>> *r)
>>>        if (!cpu_arch_flags.bitfield.cpuapx_f
>>>  	  || flag_code != CODE_64BIT)
>>>  	return false;
>>> +
>>> +      /* When using RegRex2, dual VEX/EVEX templates need to be marked as
>> EVEX.
>>> +	 For the later install_template function.  */
>>> +      if (current_templates->start->opcode_modifier.vex
>>> +	  && current_templates->start->opcode_modifier.evex)
>>> +	i.vec_encoding = vex_encoding_evex;
>>
>> I'm afraid I don't understand the 2nd sentence of the comment. This may be
>> related to my question regarding cpu_flags_match() further up.
>>
>> The first sentence isn't quite correct either - you don't mark any template here
>> (and you can't, because we don't even know yet which template we're going
>> to use).
>>
>> Finally - do you really need the .evex check here? (I won't exclude that this
>> yields a better diagnostic in certain cases, but this wants clarifying if so.)
>>
> 
> If you look at install_template(), you'll see that before this function we need to know if the current encoding is evex.

"This function" being check_register()? If so, then no, we can't know up front
whether EVEX encoding is going to be needed, as operand parsing happens ahead
of template selection. If instead you mean "that function" and hence
install_template(), then yes, we need to know whether to use EVEX there. Yet
how does that result in a need for the .evex check here? (Or maybe your reply
was really to the first of the three parts of my earlier one?)

But anyway - as said earlier on, using current_templates here looks wrong in
the first place. check_register() deals with only a register, without regard
to the context it is used in (with the sole exception of allow_pseudo_reg).
May I remind you that earlier on I already indicated that I suspect you'll
need a new enumerator to put in i.vec_encoding for this new purpose?

> We need to check opcode_modifier.evex here, it is a fix for issues caused by the merge of VEX and EVEX.
>   if (t->opcode_modifier.vex && t->opcode_modifier.evex)
>     {
>       if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
>           || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) || APX_F(CpuCMPCCXADD)
>           || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) || APX_F(CpuAVX512DQ)
>           || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
>         {
>           if (need_evex_encoding ())
>             {
>[...]
>>> @@ -1319,13 +1320,16 @@ getsec, 0xf37, SMX, NoSuf, {}
>>>
>>>  invept, 0x660f3880, EPT&No64, Modrm|IgnoreSize|NoSuf, {
>>> Oword|Unspecified|BaseIndex, Reg32 }  invept, 0x660f3880, EPT&x64,
>>> Modrm|NoSuf|NoRex64, { Oword|Unspecified|BaseIndex, Reg64 }
>>> +invept, 0xf3f0, EPT&APX_F, Modrm|NoSuf|EVex128|EVexMap4, {
>>> +Oword|Unspecified|BaseIndex, Reg64 }
>>>  invvpid, 0x660f3881, EPT&No64, Modrm|IgnoreSize|NoSuf, {
>>> Oword|Unspecified|BaseIndex, Reg32 }  invvpid, 0x660f3881, EPT&x64,
>>> Modrm|NoSuf|NoRex64, { Oword|Unspecified|BaseIndex, Reg64 }
>>> +invvpid, 0xf3f1, EPT&APX_F, Modrm|NoSuf|EVex128|EVexMap4, {
>>> +Oword|Unspecified|BaseIndex, Reg64 }
>>
>> Seeing these: Are there any Map4 encodings which aren't EVex128? If not
>> (and if you're also not hiddenly aware of some appearing in the near future),
>> please consider making EVexMap4 include this right away. Even if in the longer
>> run other encodings appear, it'll then be easy to simply replace all the
>> EVexMap4 uses in a purely mechanical way. Until then shorter template lines
>> are preferable.
>>
> 
> Would you mind defining it this way? Since #define EVex128 is behind it. Considering that you don't like unnecessary changes.
> 
> +#define EVexMap4 OpcodeSpace=SPACE_EVEXMAP4|EVex=EVEX128

The order of #define-s doesn't matter. There's no reason not to use EVex128 here
even if it's #define-d only a few lines later.

>>> @@ -1837,14 +1842,14 @@ xtest, 0xf01d6, HLE|RTM, NoSuf, {}
>>>
>>>  // BMI2 instructions.
>>>
>>> -bzhi, 0xf5, BMI2,
>>>
>> Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|SwapSources|No
>> _bSuf|No
>>> _wSuf|No_sSuf, { Reg32|Reg64, Reg32|Reg64|Unspecified|BaseIndex,
>>> Reg32|Reg64 } -mulx, 0xf2f6, BMI2,
>>>
>> Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|No_bSuf|No_wS
>> uf|No_sSu
>>> f, { Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64, Reg32|Reg64 }
>>> -pdep, 0xf2f5, BMI2,
>>>
>> Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|No_bSuf|No_wS
>> uf|No_sSu
>>> f, { Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64, Reg32|Reg64 }
>>> -pext, 0xf3f5, BMI2,
>>>
>> Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|No_bSuf|No_wS
>> uf|No_sSu
>>> f, { Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64, Reg32|Reg64 }
>>> -rorx, 0xf2f0, BMI2,
>>>
>> Modrm|CheckOperandSize|Vex128|Space0F3A|No_bSuf|No_wSuf|No_sSu
>> f, {
>>> Imm8|Imm8S, Reg32|Reg64|Dword|Qword|Unspecified|BaseIndex,
>> Reg32|Reg64
>>> } -sarx, 0xf3f7, BMI2,
>>>
>> Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|SwapSources|No
>> _bSuf|No
>>> _wSuf|No_sSuf, { Reg32|Reg64, Reg32|Reg64|Unspecified|BaseIndex,
>>> Reg32|Reg64 } -shlx, 0x66f7, BMI2,
>>>
>> Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|SwapSources|No
>> _bSuf|No
>>> _wSuf|No_sSuf, { Reg32|Reg64, Reg32|Reg64|Unspecified|BaseIndex,
>>> Reg32|Reg64 } -shrx, 0xf2f7, BMI2,
>>>
>> Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|SwapSources|No
>> _bSuf|No
>>> _wSuf|No_sSuf, { Reg32|Reg64, Reg32|Reg64|Unspecified|BaseIndex,
>>> Reg32|Reg64 }
>>> +bzhi, 0xf5, BMI2&(BMI2|APX_F),
>>>
>> +Modrm|CheckOperandSize|Vex128|EVex128|Space0F38|VexVVVV|SwapS
>> ources|N
>>> +o_bSuf|No_wSuf|No_sSuf|NF, { Reg32|Reg64,
>>> +Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64 }
>>
>> Hmm, I had specifically suggested a pre-processor macro to use in place of the
>> open-coded BMI2&(BMI2|APX_F). Is there a reason you didn't use that (here
>> and below)?
> 
> There are many different types of combinations, and each combination appears relatively few times, so I think adding a #define for each combination feels a bit wasteful.

I never suggested using multiple #define-s. I suggested a single APX_F()
macro which would be used uniformly here and elsewhere (here: APX_F(BMI2)).
And that macro would come with a comment explaining why the expression is
the (seemingly strange) way it is. Right now there's no such explanation
anywhere, and it would also be hard to find a good (central) place where to
put it.

Jan
  

On 11.12.2023 07:16, Cui, Lili wrote:
>> On 24.11.2023 08:02, Cui, Lili wrote:
>>> +	 the lower 2 bits of EVEX.aaa must be 0.  */
>>> +      if ((ins->vex.mask_register_specifier & 0x3) != 0
>>> +	  || ins->vex.ll != 0
>>> +	  || ins->vex.zeroing != 0
>>> +	  || ins->vex.b)
>>> +	return &bad_opcode;
>>> +
>>> +      /* Fall through.  */
>>>      case USE_X86_64_TABLE:
>>
>> Instead of falling through here to go through x86_64_table[] (where in all
>> cases the non-64-bit slot is "bad"), can't you avoid that step and go to the
>> next step (uniformly the LEN one) right away, saving all those new table entries
>> (along the lines of what you do below when processing into
>> evex_from_legacy)?
>>
> 
> It's not very clear to me here, do you want to add the vex_len_table to delete all entries in i386-dis-evex-x86-64.h?

Indeed I think that nothing there is really needed / warranted. The case can
be handled with no new table entries at all, I think.

>  but in this way, there are still some instructions that need to go through x86_64_table[], such as X86_64_VEX_0F38E*.

Why would these need special treatment? All EVEX-from-VEX encodings are uniform
in being defined for 64-bit code only.

>>> @@ -9041,12 +9106,24 @@ get_valid_dis386 (const struct dis386 *dp,
>>> instr_info *ins)
>>>
>>>        if (ins->address_mode != mode_64bit)
>>>  	{
>>> +	  if (ins->evex_type != evex_default
>>> +	      || (ins->rex2 & (REX_B | REX_X)))
>>> +	    return &bad_opcode;
>>
>> What's special about X and B?
>>
> 
> For evex_default, the values of these two bits are fixed. Comment added.
> 
>       if (ins->address_mode != mode_64bit)
>         {
>           /* Report bad for !evex_default and when two fixed values of evex
>              change..  */
>           if (ins->evex_type != evex_default
>               || (ins->rex2 & (REX_B | REX_X)))
>             return &bad_opcode;

Maybe you didn't get my point: What's wrong with just checking ins->rex2 here
as a whole, rather than specially treating two of the bits?

>>> @@ -9639,6 +9723,24 @@ print_insn (bfd_vma pc, disassemble_info *info,
>> int intel_syntax)
>>>        if (ins.last_repnz_prefix >= 0)
>>>  	ins.all_prefixes[ins.last_repnz_prefix] = 0xf2;
>>>        break;
>>> +
>>> +    case PREFIX_NP_OR_DATA:
>>> +      if (ins.vex.prefix & ~DATA_PREFIX_OPCODE)
>>
>> ~DATA_PREFIX_OPCODE == 0x99, which likely isn't what you mean here? Do
>> you perhaps mean e.g. "> DATA_PREFIX_OPCODE"? (Using the opcodes in
>> vex.prefix is questionable anyway, but that's a pre-existing oddity.)
>>
> 
> (A || 0) & ~A must be 0. It's hard to read.  
> 
> How about this ? This is more intuitive and easy to understand.
> 
>     case PREFIX_NP_OR_DATA:
>       if (ins.vex.prefix == REPE_PREFIX_OPCODE
>           || ins.vex.prefix == REPNE_PREFIX_OPCODE)
>         {
>           i386_dis_printf (info, dis_style_text, "(bad)");
>           ret = ins.end_codep - priv.the_buffer;
>           goto out;
>         }

That's fine with me as well, sure.

Jan
  

On 24.11.2023 08:02, Cui, Lili wrote:
> This patch adds non-ND, non-NF forms of EVEX promotion insn.
> 
> EVEX extension of legacy instructions:
>   All promoted legacy instructions are placed in EVEX map 4, which is
>   currently reserved.
> EVEX extension of EVEX instructions:
>   All existing EVEX instructions are extended by APX using the extended
>   EVEX prefix, so that they can access all 32 GPRs.
> EVEX extension of VEX instructions:
>   Promoting a VEX instruction into the EVEX space does not change the map
>   id, the opcode, or the operand encoding of the VEX instruction.
> 
> Note: The promoted versions of MOVBE will be extended to include the “MOVBE
>   reg1, reg2”.
> 
>   gas/ChangeLog:
> 
>   2023-11-21  Lingling Kong <lingling.kong@intel.com>
> 	      H.J. Lu  <hongjiu.lu@intel.com>
> 	      Lili Cui <lili.cui@intel.com>
> 	      Lin Hu   <lin1.hu@intel.com>
> 
> 	* config/tc-i386.c (cpu_flags_not_or_check): Add a new
> 	function for APX cpu flag checking.
> 	(cpu_flags_match): handle cpu_flags_not_or_check.
> 	(install_template): Add AMX_TILE and APX combine.
> 	(is_any_apx_evex_encoding): Test apx evex encoding.
> 	(build_apx_evex_prefix): Enabe APX evex prefix.
> 	(md_assemble): Handle apx with evex encoding.
> 	(check_EgprOperands): Add nodgpr check for apx.

Btw - these mechanical ChangeLog entries also need keeping up to date. Afaics
check_EgprOperands() isn't touched here (anymore?) at all, as I merely happened
to notice by searching for where the function first appears.

Jan
  

> >> On 24.11.2023 08:02, Cui, Lili wrote:
> >>> @@ -3670,10 +3673,11 @@ install_template (const insn_template *t)
> >>>
> >>>    /* Dual VEX/EVEX templates need stripping one of the possible
> variants.  */
> >>>    if (t->opcode_modifier.vex && t->opcode_modifier.evex)
> >>> -  {
> >>> -      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
> >>> -	   || maybe_cpu (t, CpuFMA))
> >>> -	  && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL)))
> >>> +    {
> >>> +      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
> >>> +	  || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
> >> APX_F(CpuCMPCCXADD)
> >>> +	  || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
> >> APX_F(CpuAVX512DQ)
> >>> +	  || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
> >>>  	{
> >>>  	  if (need_evex_encoding ())
> >>
> >> There are several issues here:
> >> - Why did you need to change (to the worse) the original code?
> >> - Why did you not model the addition after that original code?
> >> - How come APX_F (CpuAVX512*) constructs appear here, when no
> AVX512
> >> insn can be VEX-encoded?
> >
> >  I don't understand what you mean, we have this combination.
> >
> > kmov<dq>, 0x<dq:kpfx>90, AVX512BW&(AVX512BW|APX_F),
> > Modrm|Vex128|EVex128|Space0F|VexW1|<dq:kvsz>|NoSuf, {
> > RegMask|<dq:elem>|Unspecified|BaseIndex, RegMask }
> 
> Oh, I'm sorry: I forgot about the mask register insns.
> 
> >> - If these new macros are really needed for whatever reason, they
> shouldn't
> >>   be added to opcodes/i386-opc.h when they're useful only in the
> assembler.
> >> - Style requires a blank before the opening parenthesis in function
> >>   invocations (which also covers function-like macro invocations).
> >>
> >> I think I asked before: How is it that you get away without altering
> >> cpu_flags_match(), containing related and quite similar logic?
> >>
> >
> > For the original logic ( ... || ... ) && ( ... || ...), the content in the first bracket
> and the content in the following brackets can be combined arbitrarily. I think
> it is Inaccurate.
> 
> In which way? If there are issues with the existing code, these issues want
> taking care of in separate (prereq) patches. Of course there are assumptions
> made here about the CPU combinations that can (and cannot) occur in any of
> our templates. Similar assumptions are imo fine to make in the APX additions.
> 
> Note how I used two nested if()s despite that not having been necessary at
> that time. I did so in anticipation that for APX you'd want to add another
> (separate) inner if(), rather than altering the one that's there.
> 
Hi Jan, 

Could we remove the CPU check here? it's a bit ugly and has limited effectiveness.

  if (t->opcode_modifier.vex && t->opcode_modifier.evex)
    {
      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
          || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) || APX_F(CpuCMPCCXADD)
          || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) || APX_F(CpuAVX512DQ)
          || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))


> > So I give examples one by one for each identified combination.
> 
> Which examples are you talking about? I see none given in your reply.
> 

Sorry, I want to say "I've listed every possible combination".

> > Just found cpu_flags_match() has similar logic, I think the following is the
> only code related to CPUID alerts, but none of our combinations are related
> to cpuavx.
> >
> >           if (all.bitfield.cpuavx)
> >             {
> >               /* We need to check SSE2AVX with AVX.  */
> >               if (!t->opcode_modifier.sse2avx
> >                   || (sse2avx && !i.prefix[DATA_PREFIX]))
> >                 match |= CPU_FLAGS_ARCH_MATCH;
> >             }
> 
> Not sure why you pick out this one. This special case is needed for sse2avx; I
> don't see how it's related here. What I've been pointing you at is the code in
> that function which follows a similar "Dual VEX/EVEX templates ..."
> comment.
> 

I know you're talking about this code, I'm just guessing what it does? Don't know what I missed.

For example 

.arch .nobmi
andn    (%eax), %eax, %eax

---------------------------------------------------------------------------------------------
  if (flag_code != CODE_64BIT)
    active = cpu_flags_and_not (cpu_arch_flags, cpu_64_flags);
  else
    active = cpu_arch_flags;                   ---> cpubmi = 0;
  cpu = cpu_flags_and (all, active);      ---> cpuapx =1; cpubmi = 0;
  if (cpu_flags_equal (&cpu, &all))       ---> &cpu and &all are not same.
    {
    ...
    }    
Return  CPU_FLAGS_64BIT_MATCH
----------------------------------------------------------------------------------------------
Then we will report an arch error.

          if (supported != CPU_FLAGS_PERFECT_MATCH)
            {
              as_bad (_("`%s' is not supported on `%s%s'"),
                      insn_name (current_templates.start),
                      cpu_arch_name ? cpu_arch_name : default_arch,
                      cpu_sub_arch_name ? cpu_sub_arch_name : "");
              return NULL;
            }

Thanks,
Lili.
  

On 12.12.2023 11:44, Cui, Lili wrote:
>>>> On 24.11.2023 08:02, Cui, Lili wrote:
>>>>> @@ -3670,10 +3673,11 @@ install_template (const insn_template *t)
>>>>>
>>>>>    /* Dual VEX/EVEX templates need stripping one of the possible
>> variants.  */
>>>>>    if (t->opcode_modifier.vex && t->opcode_modifier.evex)
>>>>> -  {
>>>>> -      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
>>>>> -	   || maybe_cpu (t, CpuFMA))
>>>>> -	  && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL)))
>>>>> +    {
>>>>> +      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
>>>>> +	  || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
>>>> APX_F(CpuCMPCCXADD)
>>>>> +	  || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
>>>> APX_F(CpuAVX512DQ)
>>>>> +	  || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
>>>>>  	{
>>>>>  	  if (need_evex_encoding ())
>>>>
>>>> There are several issues here:
>>>> - Why did you need to change (to the worse) the original code?
>>>> - Why did you not model the addition after that original code?
>>>> - How come APX_F (CpuAVX512*) constructs appear here, when no
>> AVX512
>>>> insn can be VEX-encoded?
>>>
>>>  I don't understand what you mean, we have this combination.
>>>
>>> kmov<dq>, 0x<dq:kpfx>90, AVX512BW&(AVX512BW|APX_F),
>>> Modrm|Vex128|EVex128|Space0F|VexW1|<dq:kvsz>|NoSuf, {
>>> RegMask|<dq:elem>|Unspecified|BaseIndex, RegMask }
>>
>> Oh, I'm sorry: I forgot about the mask register insns.
>>
>>>> - If these new macros are really needed for whatever reason, they
>> shouldn't
>>>>   be added to opcodes/i386-opc.h when they're useful only in the
>> assembler.
>>>> - Style requires a blank before the opening parenthesis in function
>>>>   invocations (which also covers function-like macro invocations).
>>>>
>>>> I think I asked before: How is it that you get away without altering
>>>> cpu_flags_match(), containing related and quite similar logic?
>>>>
>>>
>>> For the original logic ( ... || ... ) && ( ... || ...), the content in the first bracket
>> and the content in the following brackets can be combined arbitrarily. I think
>> it is Inaccurate.
>>
>> In which way? If there are issues with the existing code, these issues want
>> taking care of in separate (prereq) patches. Of course there are assumptions
>> made here about the CPU combinations that can (and cannot) occur in any of
>> our templates. Similar assumptions are imo fine to make in the APX additions.
>>
>> Note how I used two nested if()s despite that not having been necessary at
>> that time. I did so in anticipation that for APX you'd want to add another
>> (separate) inner if(), rather than altering the one that's there.
> 
> Could we remove the CPU check here? it's a bit ugly and has limited effectiveness.
> 
>   if (t->opcode_modifier.vex && t->opcode_modifier.evex)
>     {
>       if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
>           || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) || APX_F(CpuCMPCCXADD)
>           || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) || APX_F(CpuAVX512DQ)
>           || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))

I agree on the "a bit ugly" part, but taking what's there right now I
don't understand "has limited effectiveness". Of course you can remove
any code you want, provided you can prove nothing breaks.

>>> So I give examples one by one for each identified combination.
>>
>> Which examples are you talking about? I see none given in your reply.
>>
> 
> Sorry, I want to say "I've listed every possible combination".
> 
>>> Just found cpu_flags_match() has similar logic, I think the following is the
>> only code related to CPUID alerts, but none of our combinations are related
>> to cpuavx.
>>>
>>>           if (all.bitfield.cpuavx)
>>>             {
>>>               /* We need to check SSE2AVX with AVX.  */
>>>               if (!t->opcode_modifier.sse2avx
>>>                   || (sse2avx && !i.prefix[DATA_PREFIX]))
>>>                 match |= CPU_FLAGS_ARCH_MATCH;
>>>             }
>>
>> Not sure why you pick out this one. This special case is needed for sse2avx; I
>> don't see how it's related here. What I've been pointing you at is the code in
>> that function which follows a similar "Dual VEX/EVEX templates ..."
>> comment.
>>
> 
> I know you're talking about this code, I'm just guessing what it does? Don't know what I missed.

You pulled out this sse2avx code. Hence I was expecting you to tell me
why you consider it relevant here.

> For example 
> 
> .arch .nobmi
> andn    (%eax), %eax, %eax
> 
> ---------------------------------------------------------------------------------------------
>   if (flag_code != CODE_64BIT)
>     active = cpu_flags_and_not (cpu_arch_flags, cpu_64_flags);
>   else
>     active = cpu_arch_flags;                   ---> cpubmi = 0;
>   cpu = cpu_flags_and (all, active);      ---> cpuapx =1; cpubmi = 0;
>   if (cpu_flags_equal (&cpu, &all))       ---> &cpu and &all are not same.
>     {
>     ...
>     }    
> Return  CPU_FLAGS_64BIT_MATCH
> ----------------------------------------------------------------------------------------------
> Then we will report an arch error.
> 
>           if (supported != CPU_FLAGS_PERFECT_MATCH)
>             {
>               as_bad (_("`%s' is not supported on `%s%s'"),
>                       insn_name (current_templates.start),
>                       cpu_arch_name ? cpu_arch_name : default_arch,
>                       cpu_sub_arch_name ? cpu_sub_arch_name : "");
>               return NULL;
>             }

Which is what we want, I think (for the particular example you picked)? Yet
again, I don't think I can see what you're trying to tell me. I also have
to confess I've lost track of whether we're discussing install_template(),
cpu_flag_match(), or both. For example in install_template() you may
indeed be able to get away with little or no changes, as long as there's
no used features tracking for APX (see the early ELF-specific part of
output_insn()). Things would be somewhat inconsistent then, but that may be
tolerable (as long as properly justified in the patch description). Not
getting this into proper shape right with the introduction of APX may bite
us later, though.

Jan
  

> >>>>> @@ -3670,10 +3673,11 @@ install_template (const insn_template *t)
> >>>>>
> >>>>>    /* Dual VEX/EVEX templates need stripping one of the possible
> >> variants.  */
> >>>>>    if (t->opcode_modifier.vex && t->opcode_modifier.evex)
> >>>>> -  {
> >>>>> -      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
> >>>>> -	   || maybe_cpu (t, CpuFMA))
> >>>>> -	  && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL)))
> >>>>> +    {
> >>>>> +      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
> >>>>> +	  || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
> >>>> APX_F(CpuCMPCCXADD)
> >>>>> +	  || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
> >>>> APX_F(CpuAVX512DQ)
> >>>>> +	  || APX_F(CpuAVX512BW) || APX_F(CpuBMI) ||
> APX_F(CpuBMI2))
> >>>>>  	{
> >>>>>  	  if (need_evex_encoding ())
> >>>>
> >>>> There are several issues here:
> >>>> - Why did you need to change (to the worse) the original code?
> >>>> - Why did you not model the addition after that original code?
> >>>> - How come APX_F (CpuAVX512*) constructs appear here, when no
> >> AVX512
> >>>> insn can be VEX-encoded?
> >>>
> >>>  I don't understand what you mean, we have this combination.
> >>>
> >>> kmov<dq>, 0x<dq:kpfx>90, AVX512BW&(AVX512BW|APX_F),
> >>> Modrm|Vex128|EVex128|Space0F|VexW1|<dq:kvsz>|NoSuf, {
> >>> RegMask|<dq:elem>|Unspecified|BaseIndex, RegMask }
> >>
> >> Oh, I'm sorry: I forgot about the mask register insns.
> >>
> >>>> - If these new macros are really needed for whatever reason, they
> >> shouldn't
> >>>>   be added to opcodes/i386-opc.h when they're useful only in the
> >> assembler.
> >>>> - Style requires a blank before the opening parenthesis in function
> >>>>   invocations (which also covers function-like macro invocations).
> >>>>
> >>>> I think I asked before: How is it that you get away without
> >>>> altering cpu_flags_match(), containing related and quite similar logic?
> >>>>
> >>>
> >>> For the original logic ( ... || ... ) && ( ... || ...), the content
> >>> in the first bracket
> >> and the content in the following brackets can be combined
> >> arbitrarily. I think it is Inaccurate.
> >>
> >> In which way? If there are issues with the existing code, these
> >> issues want taking care of in separate (prereq) patches. Of course
> >> there are assumptions made here about the CPU combinations that can
> >> (and cannot) occur in any of our templates. Similar assumptions are imo
> fine to make in the APX additions.
> >>
> >> Note how I used two nested if()s despite that not having been
> >> necessary at that time. I did so in anticipation that for APX you'd
> >> want to add another
> >> (separate) inner if(), rather than altering the one that's there.
> >
> > Could we remove the CPU check here? it's a bit ugly and has limited
> effectiveness.
> >
> >   if (t->opcode_modifier.vex && t->opcode_modifier.evex)
> >     {
> >       if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
> >           || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
> APX_F(CpuCMPCCXADD)
> >           || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
> APX_F(CpuAVX512DQ)
> >           || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
> 
> I agree on the "a bit ugly" part, but taking what's there right now I don't
> understand "has limited effectiveness". Of course you can remove any code
> you want, provided you can prove nothing breaks.
> 

Here is install_template().
All I can say is that after removing the CPU check, no test cases failed. I know it's hard to convince you to delete this place, or what do you suggest to do with this? APX requires this, otherwise the test cases will fail.

-      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
-         || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) || APX_F(CpuCMPCCXADD)
-         || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) || APX_F(CpuAVX512DQ)
-         || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
-       {

> >>> So I give examples one by one for each identified combination.
> >>
> >> Which examples are you talking about? I see none given in your reply.
> >>
> >
> > Sorry, I want to say "I've listed every possible combination".
> >
> >>> Just found cpu_flags_match() has similar logic, I think the
> >>> following is the
> >> only code related to CPUID alerts, but none of our combinations are
> >> related to cpuavx.
> >>>
> >>>           if (all.bitfield.cpuavx)
> >>>             {
> >>>               /* We need to check SSE2AVX with AVX.  */
> >>>               if (!t->opcode_modifier.sse2avx
> >>>                   || (sse2avx && !i.prefix[DATA_PREFIX]))
> >>>                 match |= CPU_FLAGS_ARCH_MATCH;
> >>>             }
> >>
> >> Not sure why you pick out this one. This special case is needed for
> >> sse2avx; I don't see how it's related here. What I've been pointing
> >> you at is the code in that function which follows a similar "Dual VEX/EVEX
> templates ..."
> >> comment.
> >>
> >
> > I know you're talking about this code, I'm just guessing what it does? Don't
> know what I missed.
> 
> You pulled out this sse2avx code. Hence I was expecting you to tell me why
> you consider it relevant here.
> 
Here is cpu_flag_match().

I rechecked the code, maybe you want to say I missed the outer loop.

      cpu = cpu_flags_and (any, active);
      if (cpu_flags_all_zero (&any) || !cpu_flags_all_zero (&cpu))
        {
          if (all.bitfield.cpuavx)
            {
              /* We need to check SSE2AVX with AVX.  */
              if (!t->opcode_modifier.sse2avx
                  || (sse2avx && !i.prefix[DATA_PREFIX]))
                match |= CPU_FLAGS_ARCH_MATCH;
            }
          else
            match |= CPU_FLAGS_ARCH_MATCH;
        }

> > For example
> >
> > .arch .nobmi
> > andn    (%eax), %eax, %eax
> >
> > ---------------------------------------------------------------------------------------------
> >   if (flag_code != CODE_64BIT)
> >     active = cpu_flags_and_not (cpu_arch_flags, cpu_64_flags);
> >   else
> >     active = cpu_arch_flags;                   ---> cpubmi = 0;
> >   cpu = cpu_flags_and (all, active);      ---> cpuapx =1; cpubmi = 0;
> >   if (cpu_flags_equal (&cpu, &all))       ---> &cpu and &all are not same.
> >     {
> >     ...
> >     }
> > Return  CPU_FLAGS_64BIT_MATCH
> > ----------------------------------------------------------------------
> > ------------------------
> > Then we will report an arch error.
> >
> >           if (supported != CPU_FLAGS_PERFECT_MATCH)
> >             {
> >               as_bad (_("`%s' is not supported on `%s%s'"),
> >                       insn_name (current_templates.start),
> >                       cpu_arch_name ? cpu_arch_name : default_arch,
> >                       cpu_sub_arch_name ? cpu_sub_arch_name : "");
> >               return NULL;
> >             }
> 
> Which is what we want, I think (for the particular example you picked)? Yet
> again, I don't think I can see what you're trying to tell me. I also have to
> confess I've lost track of whether we're discussing install_template(),
> cpu_flag_match(), or both. For example in install_template() you may indeed
> be able to get away with little or no changes, as long as there's no used
> features tracking for APX (see the early ELF-specific part of output_insn()).
> Things would be somewhat inconsistent then, but that may be tolerable (as
> long as properly justified in the patch description). Not getting this into
> proper shape right with the introduction of APX may bite us later, though.
> 

Here is cpu_flag_match().
I just want to say that for the APX part we don't need to handle it in the "Double VEX/EVEX Template...".

Thanks,
Lili.
  

On 12.12.2023 13:32, Cui, Lili wrote:
>>>>>>> @@ -3670,10 +3673,11 @@ install_template (const insn_template *t)
>>>>>>>
>>>>>>>    /* Dual VEX/EVEX templates need stripping one of the possible
>>>> variants.  */
>>>>>>>    if (t->opcode_modifier.vex && t->opcode_modifier.evex)
>>>>>>> -  {
>>>>>>> -      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
>>>>>>> -	   || maybe_cpu (t, CpuFMA))
>>>>>>> -	  && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL)))
>>>>>>> +    {
>>>>>>> +      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
>>>>>>> +	  || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
>>>>>> APX_F(CpuCMPCCXADD)
>>>>>>> +	  || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
>>>>>> APX_F(CpuAVX512DQ)
>>>>>>> +	  || APX_F(CpuAVX512BW) || APX_F(CpuBMI) ||
>> APX_F(CpuBMI2))
>>>>>>>  	{
>>>>>>>  	  if (need_evex_encoding ())
>>>>>>
>>>>>> There are several issues here:
>>>>>> - Why did you need to change (to the worse) the original code?
>>>>>> - Why did you not model the addition after that original code?
>>>>>> - How come APX_F (CpuAVX512*) constructs appear here, when no
>>>> AVX512
>>>>>> insn can be VEX-encoded?
>>>>>
>>>>>  I don't understand what you mean, we have this combination.
>>>>>
>>>>> kmov<dq>, 0x<dq:kpfx>90, AVX512BW&(AVX512BW|APX_F),
>>>>> Modrm|Vex128|EVex128|Space0F|VexW1|<dq:kvsz>|NoSuf, {
>>>>> RegMask|<dq:elem>|Unspecified|BaseIndex, RegMask }
>>>>
>>>> Oh, I'm sorry: I forgot about the mask register insns.
>>>>
>>>>>> - If these new macros are really needed for whatever reason, they
>>>> shouldn't
>>>>>>   be added to opcodes/i386-opc.h when they're useful only in the
>>>> assembler.
>>>>>> - Style requires a blank before the opening parenthesis in function
>>>>>>   invocations (which also covers function-like macro invocations).
>>>>>>
>>>>>> I think I asked before: How is it that you get away without
>>>>>> altering cpu_flags_match(), containing related and quite similar logic?
>>>>>>
>>>>>
>>>>> For the original logic ( ... || ... ) && ( ... || ...), the content
>>>>> in the first bracket
>>>> and the content in the following brackets can be combined
>>>> arbitrarily. I think it is Inaccurate.
>>>>
>>>> In which way? If there are issues with the existing code, these
>>>> issues want taking care of in separate (prereq) patches. Of course
>>>> there are assumptions made here about the CPU combinations that can
>>>> (and cannot) occur in any of our templates. Similar assumptions are imo
>> fine to make in the APX additions.
>>>>
>>>> Note how I used two nested if()s despite that not having been
>>>> necessary at that time. I did so in anticipation that for APX you'd
>>>> want to add another
>>>> (separate) inner if(), rather than altering the one that's there.
>>>
>>> Could we remove the CPU check here? it's a bit ugly and has limited
>> effectiveness.
>>>
>>>   if (t->opcode_modifier.vex && t->opcode_modifier.evex)
>>>     {
>>>       if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
>>>           || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
>> APX_F(CpuCMPCCXADD)
>>>           || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
>> APX_F(CpuAVX512DQ)
>>>           || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
>>
>> I agree on the "a bit ugly" part, but taking what's there right now I don't
>> understand "has limited effectiveness". Of course you can remove any code
>> you want, provided you can prove nothing breaks.
>>
> 
> Here is install_template().
> All I can say is that after removing the CPU check, no test cases failed. I know it's hard to convince you to delete this place, or what do you suggest to do with this? APX requires this, otherwise the test cases will fail.
> 
> -      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
> -         || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) || APX_F(CpuCMPCCXADD)
> -         || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) || APX_F(CpuAVX512DQ)
> -         || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
> -       {

So be it then (assuming you don't delete any pre-existing code there). As
said, I expect this will bite us later.

>>>>> Just found cpu_flags_match() has similar logic, I think the
>>>>> following is the
>>>> only code related to CPUID alerts, but none of our combinations are
>>>> related to cpuavx.
>>>>>
>>>>>           if (all.bitfield.cpuavx)
>>>>>             {
>>>>>               /* We need to check SSE2AVX with AVX.  */
>>>>>               if (!t->opcode_modifier.sse2avx
>>>>>                   || (sse2avx && !i.prefix[DATA_PREFIX]))
>>>>>                 match |= CPU_FLAGS_ARCH_MATCH;
>>>>>             }
>>>>
>>>> Not sure why you pick out this one. This special case is needed for
>>>> sse2avx; I don't see how it's related here. What I've been pointing
>>>> you at is the code in that function which follows a similar "Dual VEX/EVEX
>> templates ..."
>>>> comment.
>>>>
>>>
>>> I know you're talking about this code, I'm just guessing what it does? Don't
>> know what I missed.
>>
>> You pulled out this sse2avx code. Hence I was expecting you to tell me why
>> you consider it relevant here.
>>
> Here is cpu_flag_match().
> 
> I rechecked the code, maybe you want to say I missed the outer loop.
> 
>       cpu = cpu_flags_and (any, active);
>       if (cpu_flags_all_zero (&any) || !cpu_flags_all_zero (&cpu))
>         {
>           if (all.bitfield.cpuavx)
>             {
>               /* We need to check SSE2AVX with AVX.  */
>               if (!t->opcode_modifier.sse2avx
>                   || (sse2avx && !i.prefix[DATA_PREFIX]))
>                 match |= CPU_FLAGS_ARCH_MATCH;
>             }
>           else
>             match |= CPU_FLAGS_ARCH_MATCH;
>         }

No, ...

>>> For example
>>>
>>> .arch .nobmi
>>> andn    (%eax), %eax, %eax
>>>
>>> ---------------------------------------------------------------------------------------------
>>>   if (flag_code != CODE_64BIT)
>>>     active = cpu_flags_and_not (cpu_arch_flags, cpu_64_flags);
>>>   else
>>>     active = cpu_arch_flags;                   ---> cpubmi = 0;
>>>   cpu = cpu_flags_and (all, active);      ---> cpuapx =1; cpubmi = 0;
>>>   if (cpu_flags_equal (&cpu, &all))       ---> &cpu and &all are not same.
>>>     {
>>>     ...
>>>     }
>>> Return  CPU_FLAGS_64BIT_MATCH
>>> ----------------------------------------------------------------------
>>> ------------------------
>>> Then we will report an arch error.
>>>
>>>           if (supported != CPU_FLAGS_PERFECT_MATCH)
>>>             {
>>>               as_bad (_("`%s' is not supported on `%s%s'"),
>>>                       insn_name (current_templates.start),
>>>                       cpu_arch_name ? cpu_arch_name : default_arch,
>>>                       cpu_sub_arch_name ? cpu_sub_arch_name : "");
>>>               return NULL;
>>>             }
>>
>> Which is what we want, I think (for the particular example you picked)? Yet
>> again, I don't think I can see what you're trying to tell me. I also have to
>> confess I've lost track of whether we're discussing install_template(),
>> cpu_flag_match(), or both. For example in install_template() you may indeed
>> be able to get away with little or no changes, as long as there's no used
>> features tracking for APX (see the early ELF-specific part of output_insn()).
>> Things would be somewhat inconsistent then, but that may be tolerable (as
>> long as properly justified in the patch description). Not getting this into
>> proper shape right with the introduction of APX may bite us later, though.
>>
> 
> Here is cpu_flag_match().
> I just want to say that for the APX part we don't need to handle it in the "Double VEX/EVEX Template...".

... I was referring to the dual VEX/EVEX logic. I have to admit I still don't
understand how you get away without touching that, but if everything works,
all is fine of course.

Jan
  

> 
> >>> @@ -14233,6 +14276,12 @@ static bool check_register (const reg_entry
> >> *r)
> >>>        if (!cpu_arch_flags.bitfield.cpuapx_f
> >>>  	  || flag_code != CODE_64BIT)
> >>>  	return false;
> >>> +
> >>> +      /* When using RegRex2, dual VEX/EVEX templates need to be
> >>> + marked as
> >> EVEX.
> >>> +	 For the later install_template function.  */
> >>> +      if (current_templates->start->opcode_modifier.vex
> >>> +	  && current_templates->start->opcode_modifier.evex)
> >>> +	i.vec_encoding = vex_encoding_evex;
> >>
> >> I'm afraid I don't understand the 2nd sentence of the comment. This
> >> may be related to my question regarding cpu_flags_match() further up.
> >>
> >> The first sentence isn't quite correct either - you don't mark any
> >> template here (and you can't, because we don't even know yet which
> >> template we're going to use).
> >>
> >> Finally - do you really need the .evex check here? (I won't exclude
> >> that this yields a better diagnostic in certain cases, but this wants
> >> clarifying if so.)
> >>
> >
> > If you look at install_template(), you'll see that before this function we
> need to know if the current encoding is evex.
> 
> "This function" being check_register()? If so, then no, we can't know up front
> whether EVEX encoding is going to be needed, as operand parsing happens
> ahead of template selection. If instead you mean "that function" and hence
> install_template(), then yes, we need to know whether to use EVEX there.
> Yet how does that result in a need for the .evex check here? (Or maybe your
> reply was really to the first of the three parts of my earlier one?)
>

Agree with you, put them here is unreasonable. 

For example 

vtestps (%r27),%ymm6

we should report unsupported  Egpr. But without .evex check, it will report "Error: no EVEX encoding for `vtestps'"

> But anyway - as said earlier on, using current_templates here looks wrong in
> the first place. check_register() deals with only a register, without regard to
> the context it is used in (with the sole exception of allow_pseudo_reg).
> May I remind you that earlier on I already indicated that I suspect you'll need
> a new enumerator to put in i.vec_encoding for this new purpose?
> 

If we don't put it in check_register(), we need to add a for loop at the beginning of the install_template() to check RegRex2. Do you think it is okay? Or create a function for it.

for (unsigned int op = 0; op < i.operands; op++)
    {
      if (i.types[op].bitfield.class != Reg)
        continue;

      if (i.op[op].regs->reg_flags & RegRex2)
        i.vec_encoding = vex_encoding_evex;
    }

  if ((i.index_reg && (i.index_reg->reg_flags & RegRex2))
      || (i.base_reg && (i.base_reg->reg_flags & RegRex2)))
    i.vec_encoding = vex_encoding_evex; 


> > We need to check opcode_modifier.evex here, it is a fix for issues caused by
> the merge of VEX and EVEX.
> >   if (t->opcode_modifier.vex && t->opcode_modifier.evex)
> >     {
> >       if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
> >           || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
> APX_F(CpuCMPCCXADD)
> >           || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
> APX_F(CpuAVX512DQ)
> >           || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
> >         {
> >           if (need_evex_encoding ())
> >             {
> >[...]
> >>> @@ -1319,13 +1320,16 @@ getsec, 0xf37, SMX, NoSuf, {}
> >>>
> >>>  invept, 0x660f3880, EPT&No64, Modrm|IgnoreSize|NoSuf, {
> >>> Oword|Unspecified|BaseIndex, Reg32 }  invept, 0x660f3880, EPT&x64,
> >>> Modrm|NoSuf|NoRex64, { Oword|Unspecified|BaseIndex, Reg64 }
> >>> +invept, 0xf3f0, EPT&APX_F, Modrm|NoSuf|EVex128|EVexMap4, {
> >>> +Oword|Unspecified|BaseIndex, Reg64 }
> >>>  invvpid, 0x660f3881, EPT&No64, Modrm|IgnoreSize|NoSuf, {
> >>> Oword|Unspecified|BaseIndex, Reg32 }  invvpid, 0x660f3881, EPT&x64,
> >>> Modrm|NoSuf|NoRex64, { Oword|Unspecified|BaseIndex, Reg64 }
> >>> +invvpid, 0xf3f1, EPT&APX_F, Modrm|NoSuf|EVex128|EVexMap4, {
> >>> +Oword|Unspecified|BaseIndex, Reg64 }
> >>
> >> Seeing these: Are there any Map4 encodings which aren't EVex128? If
> >> not (and if you're also not hiddenly aware of some appearing in the
> >> near future), please consider making EVexMap4 include this right
> >> away. Even if in the longer run other encodings appear, it'll then be
> >> easy to simply replace all the
> >> EVexMap4 uses in a purely mechanical way. Until then shorter template
> >> lines are preferable.
> >>
> >
> > Would you mind defining it this way? Since #define EVex128 is behind it.
> Considering that you don't like unnecessary changes.
> >
> > +#define EVexMap4 OpcodeSpace=SPACE_EVEXMAP4|EVex=EVEX128
> 
> The order of #define-s doesn't matter. There's no reason not to use EVex128
> here even if it's #define-d only a few lines later.
> 

OK

#define EVex128 EVex=EVEX128
#define EVex256 EVex=EVEX256
#define EVex512 EVex=EVEX512
#define EVexLIG EVex=EVEXLIG
#define EVexDYN EVex=EVEXDYN

+#define Space0F    OpcodeSpace=SPACE_0F
+#define Space0F38  OpcodeSpace=SPACE_0F38
+#define Space0F3A  OpcodeSpace=SPACE_0F3A
+#define SpaceXOP08 OpcodeSpace=SPACE_XOP08
+#define SpaceXOP09 OpcodeSpace=SPACE_XOP09
+#define SpaceXOP0A OpcodeSpace=SPACE_XOP0A
+
+#define EVexMap4 OpcodeSpace=MAP4|EVex128
+#define EVexMap5 OpcodeSpace=SPACE_EVEXMAP5
+#define EVexMap6 OpcodeSpace=SPACE_EVEXMAP6

> Modrm|CheckOperandSize|Vex128|Space0F38|VexVVVV|SwapSources|No
> >> _bSuf|No
> >>> _wSuf|No_sSuf, { Reg32|Reg64, Reg32|Reg64|Unspecified|BaseIndex,
> >>> Reg32|Reg64 }
> >>> +bzhi, 0xf5, BMI2&(BMI2|APX_F),
> >>>
> >>
> +Modrm|CheckOperandSize|Vex128|EVex128|Space0F38|VexVVVV|SwapS
> >> ources|N
> >>> +o_bSuf|No_wSuf|No_sSuf|NF, { Reg32|Reg64,
> >>> +Reg32|Reg64|Unspecified|BaseIndex, Reg32|Reg64 }
> >>
> >> Hmm, I had specifically suggested a pre-processor macro to use in
> >> place of the open-coded BMI2&(BMI2|APX_F). Is there a reason you
> >> didn't use that (here and below)?
> >
> > There are many different types of combinations, and each combination
> appears relatively few times, so I think adding a #define for each combination
> feels a bit wasteful.
> 
> I never suggested using multiple #define-s. I suggested a single APX_F()
> macro which would be used uniformly here and elsewhere (here:
> APX_F(BMI2)).
> And that macro would come with a comment explaining why the expression
> is the (seemingly strange) way it is. Right now there's no such explanation
> anywhere, and it would also be hard to find a good (central) place where to
> put it.
> 

Oh, get you.

Thanks,
Lili.
  

> On 12.12.2023 13:32, Cui, Lili wrote:
> >>>>>>> @@ -3670,10 +3673,11 @@ install_template (const insn_template
> >>>>>>> *t)
> >>>>>>>
> >>>>>>>    /* Dual VEX/EVEX templates need stripping one of the possible
> >>>> variants.  */
> >>>>>>>    if (t->opcode_modifier.vex && t->opcode_modifier.evex)
> >>>>>>> -  {
> >>>>>>> -      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
> >>>>>>> -	   || maybe_cpu (t, CpuFMA))
> >>>>>>> -	  && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t,
> CpuAVX512VL)))
> >>>>>>> +    {
> >>>>>>> +      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) ||
> AVX512F(CpuFMA)
> >>>>>>> +	  || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
> >>>>>> APX_F(CpuCMPCCXADD)
> >>>>>>> +	  || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
> >>>>>> APX_F(CpuAVX512DQ)
> >>>>>>> +	  || APX_F(CpuAVX512BW) || APX_F(CpuBMI) ||
> >> APX_F(CpuBMI2))
> >>>>>>>  	{
> >>>>>>>  	  if (need_evex_encoding ())
> >>>>>>
> >>>>>> There are several issues here:
> >>>>>> - Why did you need to change (to the worse) the original code?
> >>>>>> - Why did you not model the addition after that original code?
> >>>>>> - How come APX_F (CpuAVX512*) constructs appear here, when no
> >>>> AVX512
> >>>>>> insn can be VEX-encoded?
> >>>>>
> >>>>>  I don't understand what you mean, we have this combination.
> >>>>>
> >>>>> kmov<dq>, 0x<dq:kpfx>90, AVX512BW&(AVX512BW|APX_F),
> >>>>> Modrm|Vex128|EVex128|Space0F|VexW1|<dq:kvsz>|NoSuf, {
> >>>>> RegMask|<dq:elem>|Unspecified|BaseIndex, RegMask }
> >>>>
> >>>> Oh, I'm sorry: I forgot about the mask register insns.
> >>>>
> >>>>>> - If these new macros are really needed for whatever reason, they
> >>>> shouldn't
> >>>>>>   be added to opcodes/i386-opc.h when they're useful only in the
> >>>> assembler.
> >>>>>> - Style requires a blank before the opening parenthesis in function
> >>>>>>   invocations (which also covers function-like macro invocations).
> >>>>>>
> >>>>>> I think I asked before: How is it that you get away without
> >>>>>> altering cpu_flags_match(), containing related and quite similar
> logic?
> >>>>>>
> >>>>>
> >>>>> For the original logic ( ... || ... ) && ( ... || ...), the
> >>>>> content in the first bracket
> >>>> and the content in the following brackets can be combined
> >>>> arbitrarily. I think it is Inaccurate.
> >>>>
> >>>> In which way? If there are issues with the existing code, these
> >>>> issues want taking care of in separate (prereq) patches. Of course
> >>>> there are assumptions made here about the CPU combinations that can
> >>>> (and cannot) occur in any of our templates. Similar assumptions are
> >>>> imo
> >> fine to make in the APX additions.
> >>>>
> >>>> Note how I used two nested if()s despite that not having been
> >>>> necessary at that time. I did so in anticipation that for APX you'd
> >>>> want to add another
> >>>> (separate) inner if(), rather than altering the one that's there.
> >>>
> >>> Could we remove the CPU check here? it's a bit ugly and has limited
> >> effectiveness.
> >>>
> >>>   if (t->opcode_modifier.vex && t->opcode_modifier.evex)
> >>>     {
> >>>       if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
> >>>           || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
> >> APX_F(CpuCMPCCXADD)
> >>>           || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
> >> APX_F(CpuAVX512DQ)
> >>>           || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
> >>
> >> I agree on the "a bit ugly" part, but taking what's there right now I
> >> don't understand "has limited effectiveness". Of course you can
> >> remove any code you want, provided you can prove nothing breaks.
> >>
> >
> > Here is install_template().
> > All I can say is that after removing the CPU check, no test cases failed. I
> know it's hard to convince you to delete this place, or what do you suggest to
> do with this? APX requires this, otherwise the test cases will fail.
> >
> > -      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
> > -         || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
> APX_F(CpuCMPCCXADD)
> > -         || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
> APX_F(CpuAVX512DQ)
> > -         || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
> > -       {
> 
> So be it then (assuming you don't delete any pre-existing code there). As said,
> I expect this will bite us later.
> 

Done.

+      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
+          || maybe_cpu (t, CpuFMA))
+         && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL))
+         || APX_F(CpuCMPCCXADD) || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F)
+         || APX_F(CpuAVX512DQ) || APX_F(CpuAVX512BW) || APX_F(CpuBMI)
+         || APX_F(CpuBMI2))

> >>>>> Just found cpu_flags_match() has similar logic, I think the
> >>>>> following is the
> >>>> only code related to CPUID alerts, but none of our combinations are
> >>>> related to cpuavx.
> >>>>>
> >>>>>           if (all.bitfield.cpuavx)
> >>>>>             {
> >>>>>               /* We need to check SSE2AVX with AVX.  */
> >>>>>               if (!t->opcode_modifier.sse2avx
> >>>>>                   || (sse2avx && !i.prefix[DATA_PREFIX]))
> >>>>>                 match |= CPU_FLAGS_ARCH_MATCH;
> >>>>>             }
> >>>>
> >>>> Not sure why you pick out this one. This special case is needed for
> >>>> sse2avx; I don't see how it's related here. What I've been pointing
> >>>> you at is the code in that function which follows a similar "Dual
> >>>> VEX/EVEX
> >> templates ..."
> >>>> comment.
> >>>>
> >>>
> >>> I know you're talking about this code, I'm just guessing what it
> >>> does? Don't
> >> know what I missed.
> >>
> >> You pulled out this sse2avx code. Hence I was expecting you to tell
> >> me why you consider it relevant here.
> >>
> > Here is cpu_flag_match().
> >
> > I rechecked the code, maybe you want to say I missed the outer loop.
> >
> >       cpu = cpu_flags_and (any, active);
> >       if (cpu_flags_all_zero (&any) || !cpu_flags_all_zero (&cpu))
> >         {
> >           if (all.bitfield.cpuavx)
> >             {
> >               /* We need to check SSE2AVX with AVX.  */
> >               if (!t->opcode_modifier.sse2avx
> >                   || (sse2avx && !i.prefix[DATA_PREFIX]))
> >                 match |= CPU_FLAGS_ARCH_MATCH;
> >             }
> >           else
> >             match |= CPU_FLAGS_ARCH_MATCH;
> >         }
> 
> No, ...
> 
> >>> For example
> >>>
> >>> .arch .nobmi
> >>> andn    (%eax), %eax, %eax
> >>>
> >>> ---------------------------------------------------------------------------------------------
> >>>   if (flag_code != CODE_64BIT)
> >>>     active = cpu_flags_and_not (cpu_arch_flags, cpu_64_flags);
> >>>   else
> >>>     active = cpu_arch_flags;                   ---> cpubmi = 0;
> >>>   cpu = cpu_flags_and (all, active);      ---> cpuapx =1; cpubmi = 0;
> >>>   if (cpu_flags_equal (&cpu, &all))       ---> &cpu and &all are not same.
> >>>     {
> >>>     ...
> >>>     }
> >>> Return  CPU_FLAGS_64BIT_MATCH
> >>> --------------------------------------------------------------------
> >>> --
> >>> ------------------------
> >>> Then we will report an arch error.
> >>>
> >>>           if (supported != CPU_FLAGS_PERFECT_MATCH)
> >>>             {
> >>>               as_bad (_("`%s' is not supported on `%s%s'"),
> >>>                       insn_name (current_templates.start),
> >>>                       cpu_arch_name ? cpu_arch_name : default_arch,
> >>>                       cpu_sub_arch_name ? cpu_sub_arch_name : "");
> >>>               return NULL;
> >>>             }
> >>
> >> Which is what we want, I think (for the particular example you
> >> picked)? Yet again, I don't think I can see what you're trying to
> >> tell me. I also have to confess I've lost track of whether we're
> >> discussing install_template(), cpu_flag_match(), or both. For example
> >> in install_template() you may indeed be able to get away with little
> >> or no changes, as long as there's no used features tracking for APX (see the
> early ELF-specific part of output_insn()).
> >> Things would be somewhat inconsistent then, but that may be tolerable
> >> (as long as properly justified in the patch description). Not getting
> >> this into proper shape right with the introduction of APX may bite us later,
> though.
> >>
> >
> > Here is cpu_flag_match().
> > I just want to say that for the APX part we don't need to handle it in the
> "Double VEX/EVEX Template...".
> 
> ... I was referring to the dual VEX/EVEX logic. I have to admit I still don't
> understand how you get away without touching that, but if everything
> works, all is fine of course.
> 

Maybe, when FMA is combined with AVX512F, if we disable FMA, but the current instruction belongs to AVX512F, there is no need to report cpu errors for it. But it's different with APX. The combination of APX and BMI requires that both are indispensable.

Thanks,
Lili.
  

On 12.12.2023 13:58, Cui, Lili wrote:
>>
>>>>> @@ -14233,6 +14276,12 @@ static bool check_register (const reg_entry
>>>> *r)
>>>>>        if (!cpu_arch_flags.bitfield.cpuapx_f
>>>>>  	  || flag_code != CODE_64BIT)
>>>>>  	return false;
>>>>> +
>>>>> +      /* When using RegRex2, dual VEX/EVEX templates need to be
>>>>> + marked as
>>>> EVEX.
>>>>> +	 For the later install_template function.  */
>>>>> +      if (current_templates->start->opcode_modifier.vex
>>>>> +	  && current_templates->start->opcode_modifier.evex)
>>>>> +	i.vec_encoding = vex_encoding_evex;
>>>>
>>>> I'm afraid I don't understand the 2nd sentence of the comment. This
>>>> may be related to my question regarding cpu_flags_match() further up.
>>>>
>>>> The first sentence isn't quite correct either - you don't mark any
>>>> template here (and you can't, because we don't even know yet which
>>>> template we're going to use).
>>>>
>>>> Finally - do you really need the .evex check here? (I won't exclude
>>>> that this yields a better diagnostic in certain cases, but this wants
>>>> clarifying if so.)
>>>>
>>>
>>> If you look at install_template(), you'll see that before this function we
>> need to know if the current encoding is evex.
>>
>> "This function" being check_register()? If so, then no, we can't know up front
>> whether EVEX encoding is going to be needed, as operand parsing happens
>> ahead of template selection. If instead you mean "that function" and hence
>> install_template(), then yes, we need to know whether to use EVEX there.
>> Yet how does that result in a need for the .evex check here? (Or maybe your
>> reply was really to the first of the three parts of my earlier one?)
>>
> 
> Agree with you, put them here is unreasonable. 
> 
> For example 
> 
> vtestps (%r27),%ymm6
> 
> we should report unsupported  Egpr. But without .evex check, it will report "Error: no EVEX encoding for `vtestps'"
> 
>> But anyway - as said earlier on, using current_templates here looks wrong in
>> the first place. check_register() deals with only a register, without regard to
>> the context it is used in (with the sole exception of allow_pseudo_reg).
>> May I remind you that earlier on I already indicated that I suspect you'll need
>> a new enumerator to put in i.vec_encoding for this new purpose?
>>
> 
> If we don't put it in check_register(), we need to add a for loop at the beginning of the install_template() to check RegRex2. Do you think it is okay? Or create a function for it.
> 
> for (unsigned int op = 0; op < i.operands; op++)
>     {
>       if (i.types[op].bitfield.class != Reg)
>         continue;
> 
>       if (i.op[op].regs->reg_flags & RegRex2)
>         i.vec_encoding = vex_encoding_evex;
>     }
> 
>   if ((i.index_reg && (i.index_reg->reg_flags & RegRex2))
>       || (i.base_reg && (i.base_reg->reg_flags & RegRex2)))
>     i.vec_encoding = vex_encoding_evex; 

As a last resort this may be an option. But until my suggestion wasn't at
least tried or demonstrated to be worse, I don't think the above would be
acceptable.

Jan
  

On 12.12.2023 14:15, Cui, Lili wrote:
>> On 12.12.2023 13:32, Cui, Lili wrote:
>>>>>>>>> @@ -3670,10 +3673,11 @@ install_template (const insn_template
>>>>>>>>> *t)
>>>>>>>>>
>>>>>>>>>    /* Dual VEX/EVEX templates need stripping one of the possible
>>>>>> variants.  */
>>>>>>>>>    if (t->opcode_modifier.vex && t->opcode_modifier.evex)
>>>>>>>>> -  {
>>>>>>>>> -      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
>>>>>>>>> -	   || maybe_cpu (t, CpuFMA))
>>>>>>>>> -	  && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t,
>> CpuAVX512VL)))
>>>>>>>>> +    {
>>>>>>>>> +      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) ||
>> AVX512F(CpuFMA)
>>>>>>>>> +	  || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
>>>>>>>> APX_F(CpuCMPCCXADD)
>>>>>>>>> +	  || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
>>>>>>>> APX_F(CpuAVX512DQ)
>>>>>>>>> +	  || APX_F(CpuAVX512BW) || APX_F(CpuBMI) ||
>>>> APX_F(CpuBMI2))
>>>>>>>>>  	{
>>>>>>>>>  	  if (need_evex_encoding ())
>>>>>>>>
>>>>>>>> There are several issues here:
>>>>>>>> - Why did you need to change (to the worse) the original code?
>>>>>>>> - Why did you not model the addition after that original code?
>>>>>>>> - How come APX_F (CpuAVX512*) constructs appear here, when no
>>>>>> AVX512
>>>>>>>> insn can be VEX-encoded?
>>>>>>>
>>>>>>>  I don't understand what you mean, we have this combination.
>>>>>>>
>>>>>>> kmov<dq>, 0x<dq:kpfx>90, AVX512BW&(AVX512BW|APX_F),
>>>>>>> Modrm|Vex128|EVex128|Space0F|VexW1|<dq:kvsz>|NoSuf, {
>>>>>>> RegMask|<dq:elem>|Unspecified|BaseIndex, RegMask }
>>>>>>
>>>>>> Oh, I'm sorry: I forgot about the mask register insns.
>>>>>>
>>>>>>>> - If these new macros are really needed for whatever reason, they
>>>>>> shouldn't
>>>>>>>>   be added to opcodes/i386-opc.h when they're useful only in the
>>>>>> assembler.
>>>>>>>> - Style requires a blank before the opening parenthesis in function
>>>>>>>>   invocations (which also covers function-like macro invocations).
>>>>>>>>
>>>>>>>> I think I asked before: How is it that you get away without
>>>>>>>> altering cpu_flags_match(), containing related and quite similar
>> logic?
>>>>>>>>
>>>>>>>
>>>>>>> For the original logic ( ... || ... ) && ( ... || ...), the
>>>>>>> content in the first bracket
>>>>>> and the content in the following brackets can be combined
>>>>>> arbitrarily. I think it is Inaccurate.
>>>>>>
>>>>>> In which way? If there are issues with the existing code, these
>>>>>> issues want taking care of in separate (prereq) patches. Of course
>>>>>> there are assumptions made here about the CPU combinations that can
>>>>>> (and cannot) occur in any of our templates. Similar assumptions are
>>>>>> imo
>>>> fine to make in the APX additions.
>>>>>>
>>>>>> Note how I used two nested if()s despite that not having been
>>>>>> necessary at that time. I did so in anticipation that for APX you'd
>>>>>> want to add another
>>>>>> (separate) inner if(), rather than altering the one that's there.
>>>>>
>>>>> Could we remove the CPU check here? it's a bit ugly and has limited
>>>> effectiveness.
>>>>>
>>>>>   if (t->opcode_modifier.vex && t->opcode_modifier.evex)
>>>>>     {
>>>>>       if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
>>>>>           || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
>>>> APX_F(CpuCMPCCXADD)
>>>>>           || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
>>>> APX_F(CpuAVX512DQ)
>>>>>           || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
>>>>
>>>> I agree on the "a bit ugly" part, but taking what's there right now I
>>>> don't understand "has limited effectiveness". Of course you can
>>>> remove any code you want, provided you can prove nothing breaks.
>>>>
>>>
>>> Here is install_template().
>>> All I can say is that after removing the CPU check, no test cases failed. I
>> know it's hard to convince you to delete this place, or what do you suggest to
>> do with this? APX requires this, otherwise the test cases will fail.
>>>
>>> -      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
>>> -         || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
>> APX_F(CpuCMPCCXADD)
>>> -         || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
>> APX_F(CpuAVX512DQ)
>>> -         || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
>>> -       {
>>
>> So be it then (assuming you don't delete any pre-existing code there). As said,
>> I expect this will bite us later.
> 
> Done.

I can't connect this with ...

> +      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
> +          || maybe_cpu (t, CpuFMA))
> +         && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL))
> +         || APX_F(CpuCMPCCXADD) || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F)
> +         || APX_F(CpuAVX512DQ) || APX_F(CpuAVX512BW) || APX_F(CpuBMI)
> +         || APX_F(CpuBMI2))

... this: You said you want to remove all the new checks. And now you say
"done" with the checks all still there? And even if I misunderstood you,
I still don't see why you'd modify the existing condition: The adjustments
made in the body of the if() aren't applicable to APX afaict. Plus there
are still the odd APX_F() uses; I'm sure I commented on that before. If
any adjustments need making for APX, you want to add a 2nd inner if()
inside the enclosing one.

Jan
  

> >>>>>>>>> @@ -3670,10 +3673,11 @@ install_template (const
> insn_template
> >>>>>>>>> *t)
> >>>>>>>>>
> >>>>>>>>>    /* Dual VEX/EVEX templates need stripping one of the
> >>>>>>>>> possible
> >>>>>> variants.  */
> >>>>>>>>>    if (t->opcode_modifier.vex && t->opcode_modifier.evex)
> >>>>>>>>> -  {
> >>>>>>>>> -      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
> >>>>>>>>> -	   || maybe_cpu (t, CpuFMA))
> >>>>>>>>> -	  && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t,
> >> CpuAVX512VL)))
> >>>>>>>>> +    {
> >>>>>>>>> +      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) ||
> >> AVX512F(CpuFMA)
> >>>>>>>>> +	  || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
> >>>>>>>> APX_F(CpuCMPCCXADD)
> >>>>>>>>> +	  || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
> >>>>>>>> APX_F(CpuAVX512DQ)
> >>>>>>>>> +	  || APX_F(CpuAVX512BW) || APX_F(CpuBMI) ||
> >>>> APX_F(CpuBMI2))
> >>>>>>>>>  	{
> >>>>>>>>>  	  if (need_evex_encoding ())
> >>>>>>>>
> >>>>>>>> There are several issues here:
> >>>>>>>> - Why did you need to change (to the worse) the original code?
> >>>>>>>> - Why did you not model the addition after that original code?
> >>>>>>>> - How come APX_F (CpuAVX512*) constructs appear here, when no
> >>>>>> AVX512
> >>>>>>>> insn can be VEX-encoded?
> >>>>>>>
> >>>>>>>  I don't understand what you mean, we have this combination.
> >>>>>>>
> >>>>>>> kmov<dq>, 0x<dq:kpfx>90, AVX512BW&(AVX512BW|APX_F),
> >>>>>>> Modrm|Vex128|EVex128|Space0F|VexW1|<dq:kvsz>|NoSuf, {
> >>>>>>> RegMask|<dq:elem>|Unspecified|BaseIndex, RegMask }
> >>>>>>
> >>>>>> Oh, I'm sorry: I forgot about the mask register insns.
> >>>>>>
> >>>>>>>> - If these new macros are really needed for whatever reason,
> >>>>>>>> they
> >>>>>> shouldn't
> >>>>>>>>   be added to opcodes/i386-opc.h when they're useful only in
> >>>>>>>> the
> >>>>>> assembler.
> >>>>>>>> - Style requires a blank before the opening parenthesis in function
> >>>>>>>>   invocations (which also covers function-like macro invocations).
> >>>>>>>>
> >>>>>>>> I think I asked before: How is it that you get away without
> >>>>>>>> altering cpu_flags_match(), containing related and quite
> >>>>>>>> similar
> >> logic?
> >>>>>>>>
> >>>>>>>
> >>>>>>> For the original logic ( ... || ... ) && ( ... || ...), the
> >>>>>>> content in the first bracket
> >>>>>> and the content in the following brackets can be combined
> >>>>>> arbitrarily. I think it is Inaccurate.
> >>>>>>
> >>>>>> In which way? If there are issues with the existing code, these
> >>>>>> issues want taking care of in separate (prereq) patches. Of
> >>>>>> course there are assumptions made here about the CPU combinations
> >>>>>> that can (and cannot) occur in any of our templates. Similar
> >>>>>> assumptions are imo
> >>>> fine to make in the APX additions.
> >>>>>>
> >>>>>> Note how I used two nested if()s despite that not having been
> >>>>>> necessary at that time. I did so in anticipation that for APX
> >>>>>> you'd want to add another
> >>>>>> (separate) inner if(), rather than altering the one that's there.
> >>>>>
> >>>>> Could we remove the CPU check here? it's a bit ugly and has
> >>>>> limited
> >>>> effectiveness.
> >>>>>
> >>>>>   if (t->opcode_modifier.vex && t->opcode_modifier.evex)
> >>>>>     {
> >>>>>       if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
> >>>>>           || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
> >>>> APX_F(CpuCMPCCXADD)
> >>>>>           || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
> >>>> APX_F(CpuAVX512DQ)
> >>>>>           || APX_F(CpuAVX512BW) || APX_F(CpuBMI) ||
> >>>>> APX_F(CpuBMI2))
> >>>>
> >>>> I agree on the "a bit ugly" part, but taking what's there right now
> >>>> I don't understand "has limited effectiveness". Of course you can
> >>>> remove any code you want, provided you can prove nothing breaks.
> >>>>
> >>>
> >>> Here is install_template().
> >>> All I can say is that after removing the CPU check, no test cases
> >>> failed. I
> >> know it's hard to convince you to delete this place, or what do you
> >> suggest to do with this? APX requires this, otherwise the test cases will fail.
> >>>
> >>> -      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
> >>> -         || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
> >> APX_F(CpuCMPCCXADD)
> >>> -         || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
> >> APX_F(CpuAVX512DQ)
> >>> -         || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
> >>> -       {
> >>
> >> So be it then (assuming you don't delete any pre-existing code
> >> there). As said, I expect this will bite us later.
> >
> > Done.
> 
> I can't connect this with ...
> 
> > +      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
> > +          || maybe_cpu (t, CpuFMA))
> > +         && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL))
> > +         || APX_F(CpuCMPCCXADD) || APX_F(CpuAMX_TILE) ||
> APX_F(CpuAVX512F)
> > +         || APX_F(CpuAVX512DQ) || APX_F(CpuAVX512BW) ||
> APX_F(CpuBMI)
> > +         || APX_F(CpuBMI2))
> 
> ... this: You said you want to remove all the new checks. And now you say
> "done" with the checks all still there? And even if I misunderstood you, I still
> don't see why you'd modify the existing condition: The adjustments made in
> the body of the if() aren't applicable to APX afaict. Plus there are still the odd
> APX_F() uses; I'm sure I commented on that before. If any adjustments need
> making for APX, you want to add a 2nd inner if() inside the enclosing one.
> 

I want to remove all, including your pre-existing code,  there is an EVEX testcase failure due to not clean i.tm.opcode_modifier.vex = 0;  As you required that don't delete any pre-existing code, so I still need to add my new combination,  

How about this ?


I want to remove all code, including your pre-existing code, VEX test case fails because it wasn't cleaned up i.tm.opcode_modifier.evex = 0; As you asked, don't remove any pre-existing code, so I still need to add my new combinations.

How about this?

  /* Dual VEX/EVEX templates need stripping one of the possible variants.  */
  if (t->opcode_modifier.vex && t->opcode_modifier.evex)
    {
      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
           || maybe_cpu (t, CpuFMA))
          && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL)))
        {
          if (need_evex_encoding ())
            {
              i.tm.opcode_modifier.vex = 0;
              i.tm.cpu.bitfield.cpuavx512f = i.tm.cpu_any.bitfield.cpuavx512f;
              i.tm.cpu.bitfield.cpuavx512vl = i.tm.cpu_any.bitfield.cpuavx512vl;
            }
          else
            {
              i.tm.opcode_modifier.evex = 0;
              if (i.tm.cpu_any.bitfield.cpuavx)
                i.tm.cpu.bitfield.cpuavx = 1;
              else if (!i.tm.cpu.bitfield.isa)
                i.tm.cpu.bitfield.isa = i.tm.cpu_any.bitfield.isa;
              else
                gas_assert (i.tm.cpu.bitfield.isa == i.tm.cpu_any.bitfield.isa);
            }
        }

      if (APX_F(CpuCMPCCXADD) || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F)
          || APX_F(CpuAVX512DQ) || APX_F(CpuAVX512BW) || APX_F(CpuBMI)
          || APX_F(CpuBMI2))
        if (need_evex_encoding ())
          i.tm.opcode_modifier.vex = 0;
        else
          i.tm.opcode_modifier.evex = 0;
    }

Thanks,
Lili.
  

On 13.12.2023 08:36, Cui, Lili wrote:
>>>>>>>>>>> @@ -3670,10 +3673,11 @@ install_template (const
>> insn_template
>>>>>>>>>>> *t)
>>>>>>>>>>>
>>>>>>>>>>>    /* Dual VEX/EVEX templates need stripping one of the
>>>>>>>>>>> possible
>>>>>>>> variants.  */
>>>>>>>>>>>    if (t->opcode_modifier.vex && t->opcode_modifier.evex)
>>>>>>>>>>> -  {
>>>>>>>>>>> -      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
>>>>>>>>>>> -	   || maybe_cpu (t, CpuFMA))
>>>>>>>>>>> -	  && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t,
>>>> CpuAVX512VL)))
>>>>>>>>>>> +    {
>>>>>>>>>>> +      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) ||
>>>> AVX512F(CpuFMA)
>>>>>>>>>>> +	  || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
>>>>>>>>>> APX_F(CpuCMPCCXADD)
>>>>>>>>>>> +	  || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
>>>>>>>>>> APX_F(CpuAVX512DQ)
>>>>>>>>>>> +	  || APX_F(CpuAVX512BW) || APX_F(CpuBMI) ||
>>>>>> APX_F(CpuBMI2))
>>>>>>>>>>>  	{
>>>>>>>>>>>  	  if (need_evex_encoding ())
>>>>>>>>>>
>>>>>>>>>> There are several issues here:
>>>>>>>>>> - Why did you need to change (to the worse) the original code?
>>>>>>>>>> - Why did you not model the addition after that original code?
>>>>>>>>>> - How come APX_F (CpuAVX512*) constructs appear here, when no
>>>>>>>> AVX512
>>>>>>>>>> insn can be VEX-encoded?
>>>>>>>>>
>>>>>>>>>  I don't understand what you mean, we have this combination.
>>>>>>>>>
>>>>>>>>> kmov<dq>, 0x<dq:kpfx>90, AVX512BW&(AVX512BW|APX_F),
>>>>>>>>> Modrm|Vex128|EVex128|Space0F|VexW1|<dq:kvsz>|NoSuf, {
>>>>>>>>> RegMask|<dq:elem>|Unspecified|BaseIndex, RegMask }
>>>>>>>>
>>>>>>>> Oh, I'm sorry: I forgot about the mask register insns.
>>>>>>>>
>>>>>>>>>> - If these new macros are really needed for whatever reason,
>>>>>>>>>> they
>>>>>>>> shouldn't
>>>>>>>>>>   be added to opcodes/i386-opc.h when they're useful only in
>>>>>>>>>> the
>>>>>>>> assembler.
>>>>>>>>>> - Style requires a blank before the opening parenthesis in function
>>>>>>>>>>   invocations (which also covers function-like macro invocations).
>>>>>>>>>>
>>>>>>>>>> I think I asked before: How is it that you get away without
>>>>>>>>>> altering cpu_flags_match(), containing related and quite
>>>>>>>>>> similar
>>>> logic?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> For the original logic ( ... || ... ) && ( ... || ...), the
>>>>>>>>> content in the first bracket
>>>>>>>> and the content in the following brackets can be combined
>>>>>>>> arbitrarily. I think it is Inaccurate.
>>>>>>>>
>>>>>>>> In which way? If there are issues with the existing code, these
>>>>>>>> issues want taking care of in separate (prereq) patches. Of
>>>>>>>> course there are assumptions made here about the CPU combinations
>>>>>>>> that can (and cannot) occur in any of our templates. Similar
>>>>>>>> assumptions are imo
>>>>>> fine to make in the APX additions.
>>>>>>>>
>>>>>>>> Note how I used two nested if()s despite that not having been
>>>>>>>> necessary at that time. I did so in anticipation that for APX
>>>>>>>> you'd want to add another
>>>>>>>> (separate) inner if(), rather than altering the one that's there.
>>>>>>>
>>>>>>> Could we remove the CPU check here? it's a bit ugly and has
>>>>>>> limited
>>>>>> effectiveness.
>>>>>>>
>>>>>>>   if (t->opcode_modifier.vex && t->opcode_modifier.evex)
>>>>>>>     {
>>>>>>>       if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
>>>>>>>           || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
>>>>>> APX_F(CpuCMPCCXADD)
>>>>>>>           || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
>>>>>> APX_F(CpuAVX512DQ)
>>>>>>>           || APX_F(CpuAVX512BW) || APX_F(CpuBMI) ||
>>>>>>> APX_F(CpuBMI2))
>>>>>>
>>>>>> I agree on the "a bit ugly" part, but taking what's there right now
>>>>>> I don't understand "has limited effectiveness". Of course you can
>>>>>> remove any code you want, provided you can prove nothing breaks.
>>>>>>
>>>>>
>>>>> Here is install_template().
>>>>> All I can say is that after removing the CPU check, no test cases
>>>>> failed. I
>>>> know it's hard to convince you to delete this place, or what do you
>>>> suggest to do with this? APX requires this, otherwise the test cases will fail.
>>>>>
>>>>> -      if (AVX512F(CpuAVX) || AVX512F(CpuAVX2) || AVX512F(CpuFMA)
>>>>> -         || AVX512VL(CpuAVX) || AVX512VL(CpuAVX2) ||
>>>> APX_F(CpuCMPCCXADD)
>>>>> -         || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F) ||
>>>> APX_F(CpuAVX512DQ)
>>>>> -         || APX_F(CpuAVX512BW) || APX_F(CpuBMI) || APX_F(CpuBMI2))
>>>>> -       {
>>>>
>>>> So be it then (assuming you don't delete any pre-existing code
>>>> there). As said, I expect this will bite us later.
>>>
>>> Done.
>>
>> I can't connect this with ...
>>
>>> +      if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
>>> +          || maybe_cpu (t, CpuFMA))
>>> +         && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL))
>>> +         || APX_F(CpuCMPCCXADD) || APX_F(CpuAMX_TILE) ||
>> APX_F(CpuAVX512F)
>>> +         || APX_F(CpuAVX512DQ) || APX_F(CpuAVX512BW) ||
>> APX_F(CpuBMI)
>>> +         || APX_F(CpuBMI2))
>>
>> ... this: You said you want to remove all the new checks. And now you say
>> "done" with the checks all still there? And even if I misunderstood you, I still
>> don't see why you'd modify the existing condition: The adjustments made in
>> the body of the if() aren't applicable to APX afaict. Plus there are still the odd
>> APX_F() uses; I'm sure I commented on that before. If any adjustments need
>> making for APX, you want to add a 2nd inner if() inside the enclosing one.
>>
> 
> I want to remove all, including your pre-existing code,  there is an EVEX testcase failure due to not clean i.tm.opcode_modifier.vex = 0;  As you required that don't delete any pre-existing code, so I still need to add my new combination,  
> 
> How about this ?
> 
> 
> I want to remove all code, including your pre-existing code, VEX test case fails because it wasn't cleaned up i.tm.opcode_modifier.evex = 0; As you asked, don't remove any pre-existing code, so I still need to add my new combinations.
> 
> How about this?
> 
>   /* Dual VEX/EVEX templates need stripping one of the possible variants.  */
>   if (t->opcode_modifier.vex && t->opcode_modifier.evex)
>     {
>       if ((maybe_cpu (t, CpuAVX) || maybe_cpu (t, CpuAVX2)
>            || maybe_cpu (t, CpuFMA))
>           && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL)))
>         {
>           if (need_evex_encoding ())
>             {
>               i.tm.opcode_modifier.vex = 0;
>               i.tm.cpu.bitfield.cpuavx512f = i.tm.cpu_any.bitfield.cpuavx512f;
>               i.tm.cpu.bitfield.cpuavx512vl = i.tm.cpu_any.bitfield.cpuavx512vl;
>             }
>           else
>             {
>               i.tm.opcode_modifier.evex = 0;
>               if (i.tm.cpu_any.bitfield.cpuavx)
>                 i.tm.cpu.bitfield.cpuavx = 1;
>               else if (!i.tm.cpu.bitfield.isa)
>                 i.tm.cpu.bitfield.isa = i.tm.cpu_any.bitfield.isa;
>               else
>                 gas_assert (i.tm.cpu.bitfield.isa == i.tm.cpu_any.bitfield.isa);
>             }
>         }
> 
>       if (APX_F(CpuCMPCCXADD) || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F)
>           || APX_F(CpuAVX512DQ) || APX_F(CpuAVX512BW) || APX_F(CpuBMI)
>           || APX_F(CpuBMI2))
>         if (need_evex_encoding ())
>           i.tm.opcode_modifier.vex = 0;
>         else
>           i.tm.opcode_modifier.evex = 0;
>     }

Something along these lines, indeed. But without APX_F(). I've just looked
it up again:

#define APX_F(cpuid) (maybe_cpu (t, CpuAPX_F) && maybe_cpu (t, cpuid))

Why would you test CpuAPX_F over and over again in the conditional? See
how the code that has been there for a little while checks each CpuXYZ
exactly once.

Plus, simply as a style remark, you want to add braces around the if/else,
to make entirely clear that the else belongs to the inner if() (iirc some
compiler versions warn about code as you have it above).

Jan
  

> >>>>> @@ -14233,6 +14276,12 @@ static bool check_register (const
> >>>>> reg_entry
> >>>> *r)
> >>>>>        if (!cpu_arch_flags.bitfield.cpuapx_f
> >>>>>  	  || flag_code != CODE_64BIT)
> >>>>>  	return false;
> >>>>> +
> >>>>> +      /* When using RegRex2, dual VEX/EVEX templates need to be
> >>>>> + marked as
> >>>> EVEX.
> >>>>> +	 For the later install_template function.  */
> >>>>> +      if (current_templates->start->opcode_modifier.vex
> >>>>> +	  && current_templates->start->opcode_modifier.evex)
> >>>>> +	i.vec_encoding = vex_encoding_evex;
> >>>>
> >>>> I'm afraid I don't understand the 2nd sentence of the comment. This
> >>>> may be related to my question regarding cpu_flags_match() further up.
> >>>>
> >>>> The first sentence isn't quite correct either - you don't mark any
> >>>> template here (and you can't, because we don't even know yet which
> >>>> template we're going to use).
> >>>>
> >>>> Finally - do you really need the .evex check here? (I won't exclude
> >>>> that this yields a better diagnostic in certain cases, but this
> >>>> wants clarifying if so.)
> >>>>
> >>>
> >>> If you look at install_template(), you'll see that before this
> >>> function we
> >> need to know if the current encoding is evex.
> >>
> >> "This function" being check_register()? If so, then no, we can't know
> >> up front whether EVEX encoding is going to be needed, as operand
> >> parsing happens ahead of template selection. If instead you mean
> >> "that function" and hence install_template(), then yes, we need to know
> whether to use EVEX there.
> >> Yet how does that result in a need for the .evex check here? (Or
> >> maybe your reply was really to the first of the three parts of my
> >> earlier one?)
> >>
> >
> > Agree with you, put them here is unreasonable.
> >
> > For example
> >
> > vtestps (%r27),%ymm6
> >
> > we should report unsupported  Egpr. But without .evex check, it will report
> "Error: no EVEX encoding for `vtestps'"
> >
> >> But anyway - as said earlier on, using current_templates here looks
> >> wrong in the first place. check_register() deals with only a
> >> register, without regard to the context it is used in (with the sole exception
> of allow_pseudo_reg).
> >> May I remind you that earlier on I already indicated that I suspect
> >> you'll need a new enumerator to put in i.vec_encoding for this new
> purpose?
> >>
> >
> > If we don't put it in check_register(), we need to add a for loop at the
> beginning of the install_template() to check RegRex2. Do you think it is okay?
> Or create a function for it.
> >
> > for (unsigned int op = 0; op < i.operands; op++)
> >     {
> >       if (i.types[op].bitfield.class != Reg)
> >         continue;
> >
> >       if (i.op[op].regs->reg_flags & RegRex2)
> >         i.vec_encoding = vex_encoding_evex;
> >     }
> >
> >   if ((i.index_reg && (i.index_reg->reg_flags & RegRex2))
> >       || (i.base_reg && (i.base_reg->reg_flags & RegRex2)))
> >     i.vec_encoding = vex_encoding_evex;
> 
> As a last resort this may be an option. But until my suggestion wasn't at least
> tried or demonstrated to be worse, I don't think the above would be
> acceptable.
> 

> >> May I remind you that earlier on I already indicated that I suspect
> >> you'll need a new enumerator to put in i.vec_encoding for this new
> purpose?

Jan, I didn't get your point, I think the enumerator vex_encoding_evex works well, the question is how to filter if Egpr is used in the current instruction, we should make a choice before install_template, whether it's an evex template or a vex template.

Thanks,
Lili
  

On 13.12.2023 09:35, Cui, Lili wrote:
>>>>>>> @@ -14233,6 +14276,12 @@ static bool check_register (const
>>>>>>> reg_entry
>>>>>> *r)
>>>>>>>        if (!cpu_arch_flags.bitfield.cpuapx_f
>>>>>>>  	  || flag_code != CODE_64BIT)
>>>>>>>  	return false;
>>>>>>> +
>>>>>>> +      /* When using RegRex2, dual VEX/EVEX templates need to be
>>>>>>> + marked as
>>>>>> EVEX.
>>>>>>> +	 For the later install_template function.  */
>>>>>>> +      if (current_templates->start->opcode_modifier.vex
>>>>>>> +	  && current_templates->start->opcode_modifier.evex)
>>>>>>> +	i.vec_encoding = vex_encoding_evex;
>>>>>>
>>>>>> I'm afraid I don't understand the 2nd sentence of the comment. This
>>>>>> may be related to my question regarding cpu_flags_match() further up.
>>>>>>
>>>>>> The first sentence isn't quite correct either - you don't mark any
>>>>>> template here (and you can't, because we don't even know yet which
>>>>>> template we're going to use).
>>>>>>
>>>>>> Finally - do you really need the .evex check here? (I won't exclude
>>>>>> that this yields a better diagnostic in certain cases, but this
>>>>>> wants clarifying if so.)
>>>>>>
>>>>>
>>>>> If you look at install_template(), you'll see that before this
>>>>> function we
>>>> need to know if the current encoding is evex.
>>>>
>>>> "This function" being check_register()? If so, then no, we can't know
>>>> up front whether EVEX encoding is going to be needed, as operand
>>>> parsing happens ahead of template selection. If instead you mean
>>>> "that function" and hence install_template(), then yes, we need to know
>> whether to use EVEX there.
>>>> Yet how does that result in a need for the .evex check here? (Or
>>>> maybe your reply was really to the first of the three parts of my
>>>> earlier one?)
>>>>
>>>
>>> Agree with you, put them here is unreasonable.
>>>
>>> For example
>>>
>>> vtestps (%r27),%ymm6
>>>
>>> we should report unsupported  Egpr. But without .evex check, it will report
>> "Error: no EVEX encoding for `vtestps'"
>>>
>>>> But anyway - as said earlier on, using current_templates here looks
>>>> wrong in the first place. check_register() deals with only a
>>>> register, without regard to the context it is used in (with the sole exception
>> of allow_pseudo_reg).
>>>> May I remind you that earlier on I already indicated that I suspect
>>>> you'll need a new enumerator to put in i.vec_encoding for this new
>> purpose?
>>>>
>>>
>>> If we don't put it in check_register(), we need to add a for loop at the
>> beginning of the install_template() to check RegRex2. Do you think it is okay?
>> Or create a function for it.
>>>
>>> for (unsigned int op = 0; op < i.operands; op++)
>>>     {
>>>       if (i.types[op].bitfield.class != Reg)
>>>         continue;
>>>
>>>       if (i.op[op].regs->reg_flags & RegRex2)
>>>         i.vec_encoding = vex_encoding_evex;
>>>     }
>>>
>>>   if ((i.index_reg && (i.index_reg->reg_flags & RegRex2))
>>>       || (i.base_reg && (i.base_reg->reg_flags & RegRex2)))
>>>     i.vec_encoding = vex_encoding_evex;
>>
>> As a last resort this may be an option. But until my suggestion wasn't at least
>> tried or demonstrated to be worse, I don't think the above would be
>> acceptable.
>>
> 
>>>> May I remind you that earlier on I already indicated that I suspect
>>>> you'll need a new enumerator to put in i.vec_encoding for this new
>> purpose?
> 
> Jan, I didn't get your point, I think the enumerator vex_encoding_evex works well, the question is how to filter if Egpr is used in the current instruction, we should make a choice before install_template, whether it's an evex template or a vex template.

Well, of course you can make the existing enumerator work, by - see above -
adding yet another loop over all operands. In a similar way it may have
been possible to avoid the introduction of vex_encoding_evex512. But it is
generally better to record the precise reason for requiring a certain
encoding / putting in place a certain restriction, i.e. going even beyond
the desire to avoid introducing new loops over all operands if at all
possible.

Here as soon as an eGPR is used, various encodings cannot be used anymore.
That's best expressed explicitly. (It might even be that such a new
enumerator would help with the REX2 encodings. Recall that I said earlier
already that both field and enumerator names aren't fully appropriate
anymore. Yet changing them isn't a priority, so we can defer that until
after all the APX work has landed.)

Jan