RE: [PATCH] Support Intel AVX10.1

["Jiang, Haochen" <haochen.jiang@intel.com>]

Let's start from the end of the patch to make sure we are on the
same page.

> > --- a/gas/testsuite/gas/i386/xmmhi32.s
> > +++ b/gas/testsuite/gas/i386/xmmhi32.s
> > @@ -26,6 +26,7 @@ xmm:
> >  	vmovdqa	ymm24, ymm0
> >
> >  	.arch .noavx512f
> > +	.arch .noavx10.1
> >  	vaddps	xmm0, xmm1, xmm8
> >  	vaddps	xmm0, xmm1, xmm16
> >  	vaddps	xmm0, xmm1, xmm24
> 
> This (and alike) addition(s) point out another issue: People may be
> using .noavx512{f,vl} to make sure they'll know if they wrongly use
> certain insns. That protection becomes void with the additions as
> you presently make them. This also relates to the first comment below
> on i386-gen.c.
> 
> > --- a/opcodes/i386-gen.c
> > +++ b/opcodes/i386-gen.c
> > @@ -168,6 +168,8 @@ static const dependency isa_dependencies[] =
> >      "AVX2" },
> >    { "FRED",
> >      "LKGS" },
> > +  { "AVX10_1",
> > +    "AVX2" },
> 
> This can't be quite right (as in: is insufficient): There's no
> restriction to the low 16 XMM/YMM registers in AVX10.1, so some of
> AVX512 is also a prereq.
> 
> To also address the earlier comment, maybe we need an artificial (i.e.
> not user selectable) feature underlying both AVX10 and AVX512? (But I
> haven't properly thought this through, so there may be issues with
> such an approach as well.)
> 

The intention is that AVX10 and AVX512 features are orthogonal and both
based on AVX2. Therefore, if AVX10 or AVX512 is enabled, the instructions
could be used. So when it comes to .no directives, we need to disable both
of them.

> > @@ -1217,7 +1220,7 @@ static void
> >  output_i386_opcode (FILE *table, const char *name, char *str,
> >  		    char *last, int lineno)
> >  {
> > -  unsigned int i, length, prefix = 0, space = 0;
> > +  unsigned int i, j, length, prefix = 0, space = 0, k = 0;
> >    char *base_opcode, *extension_opcode, *end, *ident;
> >    char *cpu_flags, *opcode_modifier, *operand_types [MAX_OPERANDS];
> >    unsigned long long opcode;
> > @@ -1315,6 +1318,20 @@ output_i386_opcode (FILE *table, const char
> *name, char *str,
> >    ident = mkident (name);
> >    fprintf (table, "  { MN_%s, 0x%0*llx%s, %u,",
> >  	   ident, 2 * (int)length, opcode, end, i);
> > +
> > +  j = strlen(ident);
> > +  /* All AVX512F based instructions are usable for AVX10.1 except
> > +     AVX512PF/ER/4FMAPS/4VNNIW/VP2INTERSECT.  */
> > +  if (strstr (cpu_flags, "AVX512")
> > +      && !strstr (cpu_flags, "AVX512PF")
> > +      && !strstr (cpu_flags, "AVX512ER")
> > +      && !strstr (cpu_flags, "4FMAPS")
> > +      && !strstr (cpu_flags, "4VNNIW")
> > +      && !strstr (cpu_flags, "VP2INTERSECT"))
> > +    {
> > +      cpu_flags = concat (cpu_flags, "|AVX10_1", NULL);
> > +      k = 1;
> > +    }
> >    free (ident);
> 
> Unless you know for sure that there aren't going to be further AVX512
> sub-features, this looks pretty fragile.

Yes, AVX512 will be frozen. All vector instructions in the future will be
under AVX10.

> 
> The doc also lists AVX10.1/256 as a possible mode (see e.g. table 1-3),
> which isn't reflected throughout the patch at all.

In AVX10 series, the vector width will be set 'globally', which means we
could not enable 512 bit for AVX10.1 while disable 512 bit for AVX10.2.

For the current implementation, we choose to enable all the size in one
version for convenience and left it to compiler (e.g. GCC) to emit the
correct instructions. I suppose compiler should take responsibility but
not gas/disassembler to check whether they are correct. 

Alternatively, we could have another bit like avx10_512bit to enable and
disable AVX10 512 bit vector size. If that it is needed is open for discussion.

Thx,
Haochen

> 
> Jan