x86: Add support for Intel AMX instructions

[Cui, Lili]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, July 2, 2020 7:22 PM
> To: Cui, Lili <lili.cui@intel.com>
> Cc: H.J. Lu <hjl.tools@gmail.com>; binutils@sourceware.org
> Subject: Re: x86: Add support for Intel AMX instructions
> 
> On 01.07.2020 16:40, Cui, Lili wrote:
> > Thank you for your careful inspection and suggestions, I update the
> > following modifications in the attachment patch.
> 
> Thanks.
> 
> > 1.  Delete the dashes, for example use "amx_tile" instead of "amx-tile".
> 
> Urgh - my comment was meant the other way around. I generally think
> dashes ought to be preferred over underscores, with the latter used only
> where the former can't be used because of (often) lexical restrictions. Yet
> then again I realize all pre-existing ones do use underscores, so perhaps it's
> fine this way.
> 
> > 2.  Add TMM register name check when AMX is disabled, or outside of
> > 64-bit mode to check_register() 3.  Modify AMX name to follow suffixes
> appear in decode order in disassembler.
> 
> I'm afraid I don't understand this one.

2. Also below I'm missing some form of adjustment to check_register() in this patch. 
%tmm<N> should not be recognized as register names (which is even more important
 in "noprefix" mode) when AMX is disabled, or outside of 64-bit mode.

3. This looks misnamed; afaict it should be REG_VEX_0F3849_P_0_W_0_M_3.
Suffixes should appear in decode order, to help the reader both follow the logic
 and look up related table entries.

Sorry , I want to express that I have fixed them, 2 and 3 are your remark on v1 of the patch.

> 
> > 4. When disable CpuAMX_TILE, other two CpuAMX_* should also be
> disabled, as amx_tile is the base feature.
> > 5.  Add Modrm to Sibmem, #define Sibmem SIB=SIBMEM|Modrm 6.
> Change
> > "0xf34B"  to "0xf34b"
> > 7.  Change" tilerelease" opcode in to 2 byte and have a value of 0x49c0.
> > 8.  Add VEX128 and VexW0 to all AMX instructions.
> 
> As a general request - please inline at least the non-generated parts of
> patches, as it is quite a bit more cumbersome to comment on attachments.
> 

I will inline it next time, thanks.

> On the actual changes: Can you really get away without e.g. also adjusting
> match_simd_size()? It would seem to me that one of
> 
> 	vaddps %tmm1,%tmm2,%tmm3
> 	tdpbssd %xmm1,%xmm2,%xmm3
> 
> may have the function wrongly return true. Overall I think you need to
> carefully go over all existing .bitfield.{x,y,z}mmword and RegSIMD uses to
> check whether they need adjustment.

OK, I will check it all over.

> 
> I see VEXOP3 is still there. Would you mind explaining why it is needed? (See
> my respective, more extensive remark on v1 of the
> patch.)

The BEXTR you mentioned is indeed the same encoding method as amx,
We will delete VEXOP3, and find a better way to handle both of them.
 
> 
> Besides my dislike for i.has_regtmm in general, as expressed before, I find
> the way you insert the code to set it somewhat odd - you put it between
> xmm and ymm, rather than either ahead of all pre-existing ones, or after.
> 

I will fix it.

> I'm also wondering whether the %rip-relative addressing check couldn't be
> arranged to live together with pre-existing ones (some MPX insns have such a
> restriction, too).
> 

I will fix it.

> You also add at least two instances of code along the lines of
> 
> @@ -8007,7 +8074,9 @@ build_modrm_byte (void)
>  		{
>  		  i386_operand_type newdisp;
> 
> -		  gas_assert (!i.tm.opcode_modifier.sib);
> +		  /* Only check for VSIB.  */
> +		  gas_assert (!i.tm.opcode_modifier.sib
> +			      || i.tm.opcode_modifier.sib == SIBMEM);
> 
> where comment and code are not fully in sync. If you only want to check for
> VSIB, then you mean
> 
> 		  gas_assert (i.tm.opcode_modifier.sib != VECSIB128
> 			      && i.tm.opcode_modifier.sib != VECSIB256
> 			      && i.tm.opcode_modifier.sib != VECSIB512);
> 
> But of course it could as well be the comment that gets changed.

You are right , I need to change the code.

> 
> Did the testcases change at all? I still don't see any checking that VEX.W or
> VEX.L being incorrectly set would result in failed disassembly. I also think you
> want to extend at least the intel-regs testcase to prove that tmm<N> get
> treated as normal symbols outside of 64-bit mode. A similar check for 64-bit
> mode with AMX disabled would also be nice, but it looks there's no pre-
> existing test that you could extend.

I will add them.

> As to void_mode you introduce in the disassembler: Is this really needed?
> Other insns not wanting any specific size (fldenv is the first example I can
> think of) look to simply use 0 in such a case.
> 

I will fix it.

> Can you please insert new enumerators at their designated places?
> For example, the various MOD_VEX_* now sit in the middle of non- VEX ones,
> when further down there already is a group of VEX ones.
> For maintainability following existing style and arrangements is really quite
> important.
> 
> Along these lines just look at
> 
> +  X86_64_VEX_0F3849_P_0_W_0_M_0_L_0,
> +  X86_64_VEX_0F3849_MOD_3_REG_0_RM_0_LEN_0,
> 
> Why two different styles even in adjacent lines? Or look at this
> 

Because the abbreviation of  "REG_0_RM_0" is " R_0_R_0" , so I used full name instead.

> @@ -1852,7 +1889,19 @@ enum
>    VEX_LEN_0F381A_P_2_M_0,
>    VEX_LEN_0F3836_P_2,
>    VEX_LEN_0F3841_P_2,
> +  LEN_VEX_0F3849_P_0_W_0_M_0,
> +  LEN_VEX_0F3849_MOD_3_REG_0_RM_0,
> +  LEN_VEX_0F3849_P_2_W_0_M_0,
> +  LEN_VEX_0F3849_P_3_W_0_M_0,
> +  LEN_VEX_0F384B_P_1_W_0_M_0,
> +  LEN_VEX_0F384B_P_2_W_0_M_0,
> +  LEN_VEX_0F384B_P_3_W_0_M_0,
>    VEX_LEN_0F385A_P_2_M_0,
> +  LEN_VEX_0F385C_P_1_W_0_M_0,
> +  LEN_VEX_0F385E_P_0_W_0_M_0,
> +  LEN_VEX_0F385E_P_1_W_0_M_0,
> +  LEN_VEX_0F385E_P_2_W_0_M_0,
> +  LEN_VEX_0F385E_P_3_W_0_M_0,
>    VEX_LEN_0F38DB_P_2,
>    VEX_LEN_0F38F2_P_0,
>    VEX_LEN_0F38F3_R_1_P_0,
> 
> You insert LEN_VEX_* when everything around is named VEX_LEN_*.
> And there's again an outlier style wise (which is also lacking a _P_<n> infix
> from the looks of it).

I will fix it.

> 
> There's also still no support for or checking of uses of %tmm8...%tmm15.
> Even worse, there are a number of "reg > 8"
> checks, when the respective arrays only have 8 entries.

Sorry, it should be "reg > 0x07", I will fix it.
From spec we only define TMM0..TMM7, so any tmm register number exceeding 7 is illegal.

Thanks, 
Lili.


> Thanks, Jan