[PATCH v3 1/2] Support Intel AMX-MOVRS

[Jiang, Haochen]

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, January 9, 2025 9:05 PM
> 
> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -1185,6 +1185,7 @@ static const arch_entry cpu_arch[] =
> >    SUBARCH (amx_transpose, AMX_TRANSPOSE, ANY_AMX_TRANSPOSE,
> false),
> >    SUBARCH (amx_tf32, AMX_TF32, ANY_AMX_TF32, false),
> >    SUBARCH (amx_fp8, AMX_FP8, ANY_AMX_FP8, false),
> > +  SUBARCH (amx_movrs, AMX_MOVRS, ANY_AMX_MOVRS, false),
> >    SUBARCH (amx_tile, AMX_TILE, ANY_AMX_TILE, false),
> >    SUBARCH (movdiri, MOVDIRI, MOVDIRI, false),
> >    SUBARCH (movdir64b, MOVDIR64B, MOVDIR64B, false),
> > @@ -2249,14 +2250,22 @@ cpu_flags_match (const insn_template *t)
> >  	      || any.bitfield.cpuavx512f || any.bitfield.cpuavx512bw
> >  	      || any.bitfield.cpuavx512dq || any.bitfield.cpuamx_tile
> >  	      || any.bitfield.cpucmpccxadd || any.bitfield.cpuuser_msr
> > -	      || any.bitfield.cpumsr_imm || any.bitfield.cpuamx_transpose))
> > +	      || any.bitfield.cpumsr_imm || any.bitfield.cpuamx_transpose
> > +	      || any.bitfield.cpuamx_movrs))
> >  	{
> >  	  /* These checks (verifying that APX_F() was properly used in the
> >  	     opcode table entry) make sure there's no need for an "else" to
> >  	     the "if()" below.  */
> >  	  gas_assert (!cpu_flags_all_zero (&all));
> >  	  cpu = cpu_flags_and (all, any);
> > -	  gas_assert (cpu_flags_equal (&cpu, &all));
> > +	  /* For APX_F extension of multiple cpuid enabled insns, we could not
> > +	     use APX_F(cpuid_A&couid_B) since the transformation could not be
> > +	     done. Instead, we will use cpuid_A && APX_F(cpuid_B), then the
> > +	     bitfield will not be match for cpu and all. Since there won't
> > +	     be much cases, here we work around those CPUIDs and do not
> > +	     need this assert check.  */
> > +	  if (!(all.bitfield.cpuamx_transpose && any.bitfield.cpuamx_movrs))
> > +	    gas_assert (cpu_flags_equal (&cpu, &all));
> 
> First: With the extensive comment, this workaround may be okay for now.
> One
> nit though: s/much/many/.
> 
> However, slightly better would be if the assertion could remain in effect
> even for these insns. Can we massage "all" (overwritten right afterwards
> for the EVEX case) and/or "any" (cleared altogether afterwards) such that
> the assertion would pass (and the resulting "cpu" still be correct)? Can
> we perhaps even arrange for the adjustment to be done in i386-gen.c? (If
> the answer is "no" for now, I can live with that. I'd then try to find
> time later to clean this up.)

From my point of view, ideally it should be addressed in i386-gen.c when
setting "any" (i.e., set AMX-TRANSPOSE for any) since it actually should set.
But setting that might be a special handling similar with here. Thus, setting
that here should be ok. I would keep the assert as-is and put
any.bitfield.cpuamx_transpose=1 for that if condition and bring it before
calculating cpu.

> 
> > @@ -4099,6 +4117,14 @@ static const struct dis386 prefix_table[][4] = {
> >      { RM_TABLE (RM_VEX_0F3849_X86_64_L_0_W_0_M_1_P_3) },
> >    },
> >
> > +  /* PREFIX_VEX_0F384A_X86_64_W_0_L_0 */
> > +  {
> > +    { Bad_Opcode },
> > +    { Bad_Opcode },
> > +    { "tileloaddrst1",	{ TMM, MVexSIBMEM }, 0 },
> > +    { "tileloaddrs",	{ TMM, MVexSIBMEM }, 0 },
> > +  },
> 
> Seeing how this doesn't need to go through mod_table[], ...
> 
> > @@ -4283,6 +4309,20 @@ static const struct dis386 prefix_table[][4] = {
> >      { "%XErorxS",		{ Gdq, Edq, Ib }, 0 },
> >    },
> >
> > +  /* PREFIX_VEX_MAP5_F8_X86_64_M_0_L_0_W_0 */
> > +  {
> > +    { "t2rpntlvwz0rs",	{ TMM, MVexSIBMEM }, 0 },
> > +    { Bad_Opcode },
> > +    { "t2rpntlvwz1rs",	{ TMM, MVexSIBMEM }, 0 },
> > +  },
> > +
> > +  /* PREFIX_VEX_MAP5_F9_X86_64_M_0_L_0_W_0 */
> > +  {
> > +    { "t2rpntlvwz0rst1",	{ TMM, MVexSIBMEM }, 0 },
> > +    { Bad_Opcode },
> > +    { "t2rpntlvwz1rst1",	{ TMM, MVexSIBMEM }, 0 },
> > +  },
> 
> ... why do these need to? MVexSIBMEM expands to OP_M after all.

Yes and similar entries in AMX-TRANSPOSE got the same issue, I will
raise another patch to fix that.

Thx,
Haochen