[PATCH 4/8] Support APX NDD

Jan Beulich jbeulich@suse.com
Thu Sep 28 07:57:43 GMT 2023
Previous message (by thread): [PATCH 4/8] Support APX NDD
Next message (by thread): [PATCH 5/8] Support APX NDD optimized encoding.
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
On 19.09.2023 17:25, Cui, Lili wrote:
> --- a/opcodes/i386-dis-evex-prefix.h
> +++ b/opcodes/i386-dis-evex-prefix.h
> @@ -353,8 +353,8 @@
>    /* PREFIX_EVEX_MAP4_66 */
>    {
>      { MOD_TABLE (MOD_EVEX_MAP4_66_PREFIX_0) },
> -    { "adoxS",	{ Gdq, Edq }, 0 },
> -    { "adcxS",	{ Gdq, Edq }, 0 },
> +    { "adoxS",	{ VexGdq, Gdq, Edq }, 0 },
> +    { "adcxS",	{ VexGdq, Gdq, Edq }, 0 },

With the OP_VEX() change moved to the earlier patch, you wouldn't need to
alter again right away what was just added there.

> --- a/opcodes/i386-dis-evex-reg.h
> +++ b/opcodes/i386-dis-evex-reg.h
> @@ -56,6 +56,105 @@
>      { "blsmskS",	{ VexGdq, Edq }, 0 },
>      { "blsiS",		{ VexGdq, Edq }, 0 },
>    },
> +  /* REG_EVEX_MAP4_80 */
> +  {
> +    { "addA",	{ VexGb, Eb, Ib }, 0 },
> +    { "orA",	{ VexGb, Eb, Ib }, 0 },
> +    { "adcA",	{ VexGb, Eb, Ib }, 0 },
> +    { "sbbA",	{ VexGb, Eb, Ib }, 0 },

Aren't these two and other adc/sbb entries required already in the earlier
patch, for consistency with what you add there on the assembler side?

> +    { "andA",	{ VexGb, Eb, Ib }, 0 },
> +    { "subA",	{ VexGb, Eb, Ib }, 0 },
> +    { "xorA",	{ VexGb, Eb, Ib }, 0 },
> +    { Bad_Opcode },
> +  },
> +  /* REG_EVEX_MAP4_81 */
> +  {
> +    { "addQ",	{ VexGv, Ev, Iv }, 0 },
> +    { "orQ",	{ VexGv, Ev, Iv }, 0 },
> +    { "adcQ",	{ VexGv, Ev, Iv }, 0 },
> +    { "sbbQ",	{ VexGv, Ev, Iv }, 0 },
> +    { "andQ",	{ VexGv, Ev, Iv }, 0 },
> +    { "subQ",	{ VexGv, Ev, Iv }, 0 },
> +    { "xorQ",	{ VexGv, Ev, Iv }, 0 },
> +    { Bad_Opcode },
> +  },
> +  /* REG_EVEX_MAP4_83 */
> +  {
> +    { "addQ",	{ VexGv, Ev, sIb }, 0 },
> +    { "orQ",	{ VexGv, Ev, sIb }, 0 },
> +    { "adcQ",	{ VexGv, Ev, sIb }, 0 },
> +    { "sbbQ",	{ VexGv, Ev, sIb }, 0 },
> +    { "andQ",	{ VexGv, Ev, sIb }, 0 },
> +    { "subQ",	{ VexGv, Ev, sIb }, 0 },
> +    { "xorQ",	{ VexGv, Ev, sIb }, 0 },
> +    { Bad_Opcode },
> +  },

While these are needed because of the differences (from existing table
entries we have) in the last entry (albeit I don't think those last
entries need to actually be spelled out; you don't spell out trailing
invalid entries further down, and we don't do so elsewhere either), ...

> +  /* REG_EVEX_MAP4_C0 */
> +  {
> +    { "rolA",	{ VexGb, Eb, Ib }, 0 },
> +    { "rorA",	{ VexGb, Eb, Ib }, 0 },
> +    { "rclA",	{ VexGb, Eb, Ib }, 0 },
> +    { "rcrA",	{ VexGb, Eb, Ib }, 0 },
> +    { "shlA",	{ VexGb, Eb, Ib }, 0 },
> +    { "shrA",	{ VexGb, Eb, Ib }, 0 },
> +    { "shlA",	{ VexGb, Eb, Ib }, 0 },
> +    { "sarA",	{ VexGb, Eb, Ib }, 0 },
> +  },
> +  /* REG_EVEX_MAP4_C1 */
> +  {
> +    { "rolQ",	{ VexGv, Ev, Ib }, 0 },
> +    { "rorQ",	{ VexGv, Ev, Ib }, 0 },
> +    { "rclQ",	{ VexGv, Ev, Ib }, 0 },
> +    { "rcrQ",	{ VexGv, Ev, Ib }, 0 },
> +    { "shlQ",	{ VexGv, Ev, Ib }, 0 },
> +    { "shrQ",	{ VexGv, Ev, Ib }, 0 },
> +    { "shlQ",	{ VexGv, Ev, Ib }, 0 },
> +    { "sarQ",	{ VexGv, Ev, Ib }, 0 },
> +  },
> +  /* REG_EVEX_MAP4_D0 */
> +  {
> +    { "rolA",	{ VexGb, Eb, I1 }, 0 },
> +    { "rorA",	{ VexGb, Eb, I1 }, 0 },
> +    { "rclA",	{ VexGb, Eb, I1 }, 0 },
> +    { "rcrA",	{ VexGb, Eb, I1 }, 0 },
> +    { "shlA",	{ VexGb, Eb, I1 }, 0 },
> +    { "shrA",	{ VexGb, Eb, I1 }, 0 },
> +    { "shlA",	{ VexGb, Eb, I1 }, 0 },
> +    { "sarA",	{ VexGb, Eb, I1 }, 0 },
> +  },
> +  /* REG_EVEX_MAP4_D1 */
> +  {
> +    { "rolQ",	{ VexGv, Ev, I1 }, 0 },
> +    { "rorQ",	{ VexGv, Ev, I1 }, 0 },
> +    { "rclQ",	{ VexGv, Ev, I1 }, 0 },
> +    { "rcrQ",	{ VexGv, Ev, I1 }, 0 },
> +    { "shlQ",	{ VexGv, Ev, I1 }, 0 },
> +    { "shrQ",	{ VexGv, Ev, I1 }, 0 },
> +    { "shlQ",	{ VexGv, Ev, I1 }, 0 },
> +    { "sarQ",	{ VexGv, Ev, I1 }, 0 },
> +  },
> +  /* REG_EVEX_MAP4_D2 */
> +  {
> +    { "rolA",	{ VexGb, Eb, CL }, 0 },
> +    { "rorA",	{ VexGb, Eb, CL }, 0 },
> +    { "rclA",	{ VexGb, Eb, CL }, 0 },
> +    { "rcrA",	{ VexGb, Eb, CL }, 0 },
> +    { "shlA",	{ VexGb, Eb, CL }, 0 },
> +    { "shrA",	{ VexGb, Eb, CL }, 0 },
> +    { "shlA",	{ VexGb, Eb, CL }, 0 },
> +    { "sarA",	{ VexGb, Eb, CL }, 0 },
> +  },
> +  /* REG_EVEX_MAP4_D3 */
> +  {
> +    { "rolQ",	{ VexGv, Ev, CL }, 0 },
> +    { "rorQ",	{ VexGv, Ev, CL }, 0 },
> +    { "rclQ",	{ VexGv, Ev, CL }, 0 },
> +    { "rcrQ",	{ VexGv, Ev, CL }, 0 },
> +    { "shlQ",	{ VexGv, Ev, CL }, 0 },
> +    { "shrQ",	{ VexGv, Ev, CL }, 0 },
> +    { "shlQ",	{ VexGv, Ev, CL }, 0 },
> +    { "sarQ",	{ VexGv, Ev, CL }, 0 },
> +  },

... do we really need all these new entries? OP_VEX() checks need_vex first
thing, so simply adjusting and then re-using the existing entries would seem
possible.

> @@ -9070,6 +9085,14 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
>  	  ins->rex &= ~REX_B;
>  	  ins->rex2 &= ~REX_R;
>  	}
> +      if (ins->evex_type == evex_from_legacy)
> +	{
> +	  if (ins->vex.ll || ins->vex.zeroing
> +	      || (!ins->vex.b && (ins->vex.register_specifier
> +				  || !ins->vex.v)))
> +	    return &bad_opcode;

Don't these checks also apply to evex_from_vex?

> +	  ins->rex |= REX_OPCODE;

This, otoh, may truly be evex_from_legacy-only (but I'm not entirely
certain).

> @@ -9080,7 +9103,7 @@ get_valid_dis386 (const struct dis386 *dp, instr_info *ins)
>  	return &err_opcode;
>  
>        /* Set vector length.  */
> -      if (ins->modrm.mod == 3 && ins->vex.b)
> +      if (ins->modrm.mod == 3 && ins->vex.b && ins->evex_type == evex_default)
>  	ins->vex.length = 512;

Down from here, still in the same function, there's another vex.b
check which I think also wants qualifying.

> @@ -10994,7 +11017,7 @@ print_displacement (instr_info *ins, bfd_signed_vma val)
>  static void
>  intel_operand_size (instr_info *ins, int bytemode, int sizeflag)
>  {
> -  if (ins->vex.b)
> +  if (ins->vex.b && ins->evex_type != evex_from_legacy)

Wouldn't this better be ins->evex_type == evex_default, ...

> @@ -11928,7 +11951,8 @@ OP_E_memory (instr_info *ins, int bytemode, int sizeflag)
>  	ins->vex.no_broadcast = true;
>  
>        if (!ins->vex.no_broadcast
> -	  && (!ins->intel_syntax || !(ins->evex_used & EVEX_len_used)))
> +	  && (!ins->intel_syntax || !(ins->evex_used & EVEX_len_used))
> +	  && ins->evex_type == evex_default)

... just like you have it here?

However, for this change, doesn't this need moving to the enclosing if()?
You should accidentally set EVEX_b_used here for APX insns.

> @@ -13280,6 +13304,14 @@ OP_VEX (instr_info *ins, int bytemode, int sizeflag ATTRIBUTE_UNUSED)
>    if (!ins->need_vex)
>      return true;
>  
> +  if (ins->evex_type == evex_from_legacy)
> +    {
> +      if (ins->vex.b)
> +	ins->evex_used |= EVEX_b_used;
> +      else
> +	 return true;
> +    }

When you reuse fields or definitions in places where their names don't
match their purpose (the field dealt with here is "nd" after all, not
"b"), a comment wants adding. There's also something odd with
indentation here, but I suppose an if/else construct isn't needed in
the first place.

Jan
Previous message (by thread): [PATCH 4/8] Support APX NDD
Next message (by thread): [PATCH 5/8] Support APX NDD optimized encoding.
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
More information about the Binutils mailing list