x86: fold AVX512-VNNI disassembler entries with AVX-VNNI ones

Message ID 7bac66be-535e-9051-d674-f2f5ba180e17@suse.com
State Accepted
Headers
Series x86: fold AVX512-VNNI disassembler entries with AVX-VNNI ones |

Checks

Context Check Description
snail/binutils-gdb-check success Github commit url

Commit Message

Jan Beulich Oct. 14, 2022, 10:22 a.m. UTC
  Make %XV also print the separating blank in the VEX case, while making
it do nothing for EVEX-encoded insns. This way the AVX-VNNI entries
can be re-used for AVX512-VNNI, at the same time fixing the lack of
EVEX.W decoding.

For the AVX-VNNI ones further make sure only VEX.66 forms are actually
decoded.
---
Irrespective of this change I continue to disagree with the arbitrary
printing of "{vex}" for the AVX-VNNI insns: If that's meant for
disambiguation purposes, then EVEX-encoded insns not using EVEX-specific
functionality by having VEX counterparts (vaddps %xmm0, %xmm0, %xmm0)
should also be prefixed by "{evex}".
  

Comments

H.J. Lu Oct. 14, 2022, 5:28 p.m. UTC | #1
On Fri, Oct 14, 2022 at 3:22 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> Make %XV also print the separating blank in the VEX case, while making
> it do nothing for EVEX-encoded insns. This way the AVX-VNNI entries
> can be re-used for AVX512-VNNI, at the same time fixing the lack of
> EVEX.W decoding.
>
> For the AVX-VNNI ones further make sure only VEX.66 forms are actually
> decoded.
> ---
> Irrespective of this change I continue to disagree with the arbitrary
> printing of "{vex}" for the AVX-VNNI insns: If that's meant for
> disambiguation purposes, then EVEX-encoded insns not using EVEX-specific
> functionality by having VEX counterparts (vaddps %xmm0, %xmm0, %xmm0)
> should also be prefixed by "{evex}".

This is done to match the assembler.  There are 3 kinds of VNNI processors:

1. AVX512-VNNI only.
2. AVX-VNNI only.
3. AVX512-VNNI and AVX-VNNI.

Since AVX512-VNNI came out first, all VNNI instructions without a prefix
are encoded as AVX512-VNNI.  The existing VNNI instructions without
a prefix, generated by compiler or hand written, are encoded with EVEX.
If one needs VNNI with VEX encoding, the {vex} prefix should be used.
This applies to any AVX extensions which come after EVEX ones, including
AVX-IFMA.

> --- a/opcodes/i386-dis.c
> +++ b/opcodes/i386-dis.c
> @@ -1755,7 +1755,7 @@ struct dis386 {
>     "XD" => print 'd' if !EVEX or EVEX.W=1, EVEX.W=0 is not a valid encoding
>     "XH" => print 'h' if EVEX.W=0, EVEX.W=1 is not a valid encoding (for FP16)
>     "XS" => print 's' if !EVEX or EVEX.W=0, EVEX.W=1 is not a valid encoding
> -   "XV" => print "{vex3}" pseudo prefix
> +   "XV" => print "{vex} " pseudo prefix
>     "LQ" => print 'l' ('d' in Intel mode) or 'q' for memory operand, cond
>            being false, or no operand at all in 64bit mode, or if suffix_always
>            is true.
> @@ -7545,19 +7545,19 @@ static const struct dis386 vex_w_table[]
>    },
>    {
>      /* VEX_W_0F3850 */
> -    { "%XV vpdpbusd",  { XM, Vex, EXx }, 0 },
> +    { "%XVvpdpbusd",   { XM, Vex, EXx }, PREFIX_DATA },
>    },
>    {
>      /* VEX_W_0F3851 */
> -    { "%XV vpdpbusds", { XM, Vex, EXx }, 0 },
> +    { "%XVvpdpbusds",  { XM, Vex, EXx }, PREFIX_DATA },
>    },
>    {
>      /* VEX_W_0F3852 */
> -    { "%XV vpdpwssd",  { XM, Vex, EXx }, 0 },
> +    { "%XVvpdpwssd",   { XM, Vex, EXx }, PREFIX_DATA },
>    },
>    {
>      /* VEX_W_0F3853 */
> -    { "%XV vpdpwssds", { XM, Vex, EXx }, 0 },
> +    { "%XVvpdpwssds",  { XM, Vex, EXx }, PREFIX_DATA },
>    },
>    {
>      /* VEX_W_0F3858 */
> @@ -10711,22 +10711,29 @@ putop (instr_info *ins, const char *in_t
>         case 'V':
>           if (l == 0)
>             abort ();
> -         else if (l == 1
> -                  && (last[0] == 'L' || last[0] == 'X'))
> +         else if (l == 1)
>             {
> -             if (last[0] == 'X')
> +             switch (last[0])
>                 {
> +               case 'X':
> +                 if (ins->vex.evex)
> +                   break;
>                   *ins->obufp++ = '{';
>                   *ins->obufp++ = 'v';
>                   *ins->obufp++ = 'e';
>                   *ins->obufp++ = 'x';
>                   *ins->obufp++ = '}';
> -               }
> -             else if (ins->rex & REX_W)
> -               {
> +                 *ins->obufp++ = ' ';
> +                 break;
> +               case 'L':
> +                 if (!(ins->rex & REX_W))
> +                   break;
>                   *ins->obufp++ = 'a';
>                   *ins->obufp++ = 'b';
>                   *ins->obufp++ = 's';
> +                 break;
> +               default:
> +                 abort ();
>                 }
>             }
>           else
> --- a/opcodes/i386-dis-evex.h
> +++ b/opcodes/i386-dis-evex.h
> @@ -383,8 +383,8 @@ static const struct dis386 evex_table[][
>      { "vrsqrt14p%XW",  { XM, EXx }, 0 },
>      { "vrsqrt14s%XW",  { XMScalar, VexScalar, EXdq }, PREFIX_DATA },
>      /* 50 */
> -    { "vpdpbusd",      { XM, Vex, EXx }, PREFIX_DATA },
> -    { "vpdpbusds",     { XM, Vex, EXx }, PREFIX_DATA },
> +    { VEX_W_TABLE (VEX_W_0F3850) },
> +    { VEX_W_TABLE (VEX_W_0F3851) },
>      { PREFIX_TABLE (PREFIX_EVEX_0F3852) },
>      { PREFIX_TABLE (PREFIX_EVEX_0F3853) },
>      { "vpopcnt%BW",    { XM, EXx }, PREFIX_DATA },
> --- a/opcodes/i386-dis-evex-prefix.h
> +++ b/opcodes/i386-dis-evex-prefix.h
> @@ -233,14 +233,14 @@
>    {
>      { Bad_Opcode },
>      { "vdpbf16p%XS", { XM, Vex, EXx }, 0 },
> -    { "vpdpwssd",      { XM, Vex, EXx }, 0 },
> +    { VEX_W_TABLE (VEX_W_0F3852) },
>      { "vp4dpwssd",     { XM, Vex, EXxmm }, 0 },
>    },
>    /* PREFIX_EVEX_0F3853 */
>    {
>      { Bad_Opcode },
>      { Bad_Opcode },
> -    { "vpdpwssds",     { XM, Vex, EXx }, 0 },
> +    { VEX_W_TABLE (VEX_W_0F3853) },
>      { "vp4dpwssds",    { XM, Vex, EXxmm }, 0 },
>    },
>    /* PREFIX_EVEX_0F3868 */

OK.
  
Jan Beulich Oct. 17, 2022, 6:39 a.m. UTC | #2
On 14.10.2022 19:28, H.J. Lu wrote:
> On Fri, Oct 14, 2022 at 3:22 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> Make %XV also print the separating blank in the VEX case, while making
>> it do nothing for EVEX-encoded insns. This way the AVX-VNNI entries
>> can be re-used for AVX512-VNNI, at the same time fixing the lack of
>> EVEX.W decoding.
>>
>> For the AVX-VNNI ones further make sure only VEX.66 forms are actually
>> decoded.
>> ---
>> Irrespective of this change I continue to disagree with the arbitrary
>> printing of "{vex}" for the AVX-VNNI insns: If that's meant for
>> disambiguation purposes, then EVEX-encoded insns not using EVEX-specific
>> functionality by having VEX counterparts (vaddps %xmm0, %xmm0, %xmm0)
>> should also be prefixed by "{evex}".
> 
> This is done to match the assembler.  There are 3 kinds of VNNI processors:
> 
> 1. AVX512-VNNI only.
> 2. AVX-VNNI only.
> 3. AVX512-VNNI and AVX-VNNI.
> 
> Since AVX512-VNNI came out first, all VNNI instructions without a prefix
> are encoded as AVX512-VNNI.  The existing VNNI instructions without
> a prefix, generated by compiler or hand written, are encoded with EVEX.
> If one needs VNNI with VEX encoding, the {vex} prefix should be used.

... if, as said, AVX512 wasn't turned off altogether.

With your model, just look at how odd code using both AVX-VNNI and
AVX-VNNI-INT8 then looks:

	vpdpbssd	%ymm0, %ymm5, %ymm6
	vpdpbsud	%ymm1, %ymm5, %ymm6
	{vex} vpdpbusd	%ymm2, %ymm5, %ymm6
	vpdpbuud	%ymm3, %ymm5, %ymm6

Yes, one could further clutter this and add {vex} to every line. But
why would anyone want to clutter their code?

Plus the same argument then applies to AVX512VL: This came out later than
AVX, and the assembler (necessarily) requires {evex} to actually encode
these (when there are AVX equivalents). Hence to match the assembler, the
disassembler then also ought to emit {evex} for this subset of encodings.

Jan

> This applies to any AVX extensions which come after EVEX ones, including
> AVX-IFMA.
  
H.J. Lu Oct. 17, 2022, 9:10 p.m. UTC | #3
On Sun, Oct 16, 2022 at 11:39 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 14.10.2022 19:28, H.J. Lu wrote:
> > On Fri, Oct 14, 2022 at 3:22 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> Make %XV also print the separating blank in the VEX case, while making
> >> it do nothing for EVEX-encoded insns. This way the AVX-VNNI entries
> >> can be re-used for AVX512-VNNI, at the same time fixing the lack of
> >> EVEX.W decoding.
> >>
> >> For the AVX-VNNI ones further make sure only VEX.66 forms are actually
> >> decoded.
> >> ---
> >> Irrespective of this change I continue to disagree with the arbitrary
> >> printing of "{vex}" for the AVX-VNNI insns: If that's meant for
> >> disambiguation purposes, then EVEX-encoded insns not using EVEX-specific
> >> functionality by having VEX counterparts (vaddps %xmm0, %xmm0, %xmm0)
> >> should also be prefixed by "{evex}".
> >
> > This is done to match the assembler.  There are 3 kinds of VNNI processors:
> >
> > 1. AVX512-VNNI only.
> > 2. AVX-VNNI only.
> > 3. AVX512-VNNI and AVX-VNNI.
> >
> > Since AVX512-VNNI came out first, all VNNI instructions without a prefix
> > are encoded as AVX512-VNNI.  The existing VNNI instructions without
> > a prefix, generated by compiler or hand written, are encoded with EVEX.
> > If one needs VNNI with VEX encoding, the {vex} prefix should be used.
>
> ... if, as said, AVX512 wasn't turned off altogether.
>
> With your model, just look at how odd code using both AVX-VNNI and
> AVX-VNNI-INT8 then looks:
>
>         vpdpbssd        %ymm0, %ymm5, %ymm6
>         vpdpbsud        %ymm1, %ymm5, %ymm6
>         {vex} vpdpbusd  %ymm2, %ymm5, %ymm6
>         vpdpbuud        %ymm3, %ymm5, %ymm6
>
> Yes, one could further clutter this and add {vex} to every line. But
> why would anyone want to clutter their code?

This ensures that VNNI instructions without a prefix are encoded
with EVEX.

> Plus the same argument then applies to AVX512VL: This came out later than
> AVX, and the assembler (necessarily) requires {evex} to actually encode
> these (when there are AVX equivalents). Hence to match the assembler, the
> disassembler then also ought to emit {evex} for this subset of encodings.

A patch is welcome.

> Jan
>
> > This applies to any AVX extensions which come after EVEX ones, including
> > AVX-IFMA.
>
  
Jan Beulich Oct. 18, 2022, 5:41 a.m. UTC | #4
On 17.10.2022 23:10, H.J. Lu wrote:
> On Sun, Oct 16, 2022 at 11:39 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 14.10.2022 19:28, H.J. Lu wrote:
>>> On Fri, Oct 14, 2022 at 3:22 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> Make %XV also print the separating blank in the VEX case, while making
>>>> it do nothing for EVEX-encoded insns. This way the AVX-VNNI entries
>>>> can be re-used for AVX512-VNNI, at the same time fixing the lack of
>>>> EVEX.W decoding.
>>>>
>>>> For the AVX-VNNI ones further make sure only VEX.66 forms are actually
>>>> decoded.
>>>> ---
>>>> Irrespective of this change I continue to disagree with the arbitrary
>>>> printing of "{vex}" for the AVX-VNNI insns: If that's meant for
>>>> disambiguation purposes, then EVEX-encoded insns not using EVEX-specific
>>>> functionality by having VEX counterparts (vaddps %xmm0, %xmm0, %xmm0)
>>>> should also be prefixed by "{evex}".
>>>
>>> This is done to match the assembler.  There are 3 kinds of VNNI processors:
>>>
>>> 1. AVX512-VNNI only.
>>> 2. AVX-VNNI only.
>>> 3. AVX512-VNNI and AVX-VNNI.
>>>
>>> Since AVX512-VNNI came out first, all VNNI instructions without a prefix
>>> are encoded as AVX512-VNNI.  The existing VNNI instructions without
>>> a prefix, generated by compiler or hand written, are encoded with EVEX.
>>> If one needs VNNI with VEX encoding, the {vex} prefix should be used.
>>
>> ... if, as said, AVX512 wasn't turned off altogether.
>>
>> With your model, just look at how odd code using both AVX-VNNI and
>> AVX-VNNI-INT8 then looks:
>>
>>         vpdpbssd        %ymm0, %ymm5, %ymm6
>>         vpdpbsud        %ymm1, %ymm5, %ymm6
>>         {vex} vpdpbusd  %ymm2, %ymm5, %ymm6
>>         vpdpbuud        %ymm3, %ymm5, %ymm6
>>
>> Yes, one could further clutter this and add {vex} to every line. But
>> why would anyone want to clutter their code?
> 
> This ensures that VNNI instructions without a prefix are encoded
> with EVEX.

As does ".arch .noavx512f".

>> Plus the same argument then applies to AVX512VL: This came out later than
>> AVX, and the assembler (necessarily) requires {evex} to actually encode
>> these (when there are AVX equivalents). Hence to match the assembler, the
>> disassembler then also ought to emit {evex} for this subset of encodings.
> 
> A patch is welcome.

Will add to my list. That'll be a huge patch, I guess, because of the
sometimes excessive testcases that were added for the various sub-features.

Jan
  

Patch

--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -1755,7 +1755,7 @@  struct dis386 {
    "XD" => print 'd' if !EVEX or EVEX.W=1, EVEX.W=0 is not a valid encoding
    "XH" => print 'h' if EVEX.W=0, EVEX.W=1 is not a valid encoding (for FP16)
    "XS" => print 's' if !EVEX or EVEX.W=0, EVEX.W=1 is not a valid encoding
-   "XV" => print "{vex3}" pseudo prefix
+   "XV" => print "{vex} " pseudo prefix
    "LQ" => print 'l' ('d' in Intel mode) or 'q' for memory operand, cond
 	   being false, or no operand at all in 64bit mode, or if suffix_always
 	   is true.
@@ -7545,19 +7545,19 @@  static const struct dis386 vex_w_table[]
   },
   {
     /* VEX_W_0F3850 */
-    { "%XV vpdpbusd",	{ XM, Vex, EXx }, 0 },
+    { "%XVvpdpbusd",	{ XM, Vex, EXx }, PREFIX_DATA },
   },
   {
     /* VEX_W_0F3851 */
-    { "%XV vpdpbusds",	{ XM, Vex, EXx }, 0 },
+    { "%XVvpdpbusds",	{ XM, Vex, EXx }, PREFIX_DATA },
   },
   {
     /* VEX_W_0F3852 */
-    { "%XV vpdpwssd",	{ XM, Vex, EXx }, 0 },
+    { "%XVvpdpwssd",	{ XM, Vex, EXx }, PREFIX_DATA },
   },
   {
     /* VEX_W_0F3853 */
-    { "%XV vpdpwssds",	{ XM, Vex, EXx }, 0 },
+    { "%XVvpdpwssds",	{ XM, Vex, EXx }, PREFIX_DATA },
   },
   {
     /* VEX_W_0F3858 */
@@ -10711,22 +10711,29 @@  putop (instr_info *ins, const char *in_t
 	case 'V':
 	  if (l == 0)
 	    abort ();
-	  else if (l == 1
-		   && (last[0] == 'L' || last[0] == 'X'))
+	  else if (l == 1)
 	    {
-	      if (last[0] == 'X')
+	      switch (last[0])
 		{
+		case 'X':
+		  if (ins->vex.evex)
+		    break;
 		  *ins->obufp++ = '{';
 		  *ins->obufp++ = 'v';
 		  *ins->obufp++ = 'e';
 		  *ins->obufp++ = 'x';
 		  *ins->obufp++ = '}';
-		}
-	      else if (ins->rex & REX_W)
-		{
+		  *ins->obufp++ = ' ';
+		  break;
+		case 'L':
+		  if (!(ins->rex & REX_W))
+		    break;
 		  *ins->obufp++ = 'a';
 		  *ins->obufp++ = 'b';
 		  *ins->obufp++ = 's';
+		  break;
+		default:
+		  abort ();
 		}
 	    }
 	  else
--- a/opcodes/i386-dis-evex.h
+++ b/opcodes/i386-dis-evex.h
@@ -383,8 +383,8 @@  static const struct dis386 evex_table[][
     { "vrsqrt14p%XW",	{ XM, EXx }, 0 },
     { "vrsqrt14s%XW",	{ XMScalar, VexScalar, EXdq }, PREFIX_DATA },
     /* 50 */
-    { "vpdpbusd",	{ XM, Vex, EXx }, PREFIX_DATA },
-    { "vpdpbusds",	{ XM, Vex, EXx }, PREFIX_DATA },
+    { VEX_W_TABLE (VEX_W_0F3850) },
+    { VEX_W_TABLE (VEX_W_0F3851) },
     { PREFIX_TABLE (PREFIX_EVEX_0F3852) },
     { PREFIX_TABLE (PREFIX_EVEX_0F3853) },
     { "vpopcnt%BW",	{ XM, EXx }, PREFIX_DATA },
--- a/opcodes/i386-dis-evex-prefix.h
+++ b/opcodes/i386-dis-evex-prefix.h
@@ -233,14 +233,14 @@ 
   {
     { Bad_Opcode },
     { "vdpbf16p%XS", { XM, Vex, EXx }, 0 },
-    { "vpdpwssd",	{ XM, Vex, EXx }, 0 },
+    { VEX_W_TABLE (VEX_W_0F3852) },
     { "vp4dpwssd",	{ XM, Vex, EXxmm }, 0 },
   },
   /* PREFIX_EVEX_0F3853 */
   {
     { Bad_Opcode },
     { Bad_Opcode },
-    { "vpdpwssds",	{ XM, Vex, EXx }, 0 },
+    { VEX_W_TABLE (VEX_W_0F3853) },
     { "vp4dpwssds",	{ XM, Vex, EXxmm }, 0 },
   },
   /* PREFIX_EVEX_0F3868 */