x86: Add support for Intel AMX instructions

[Jan Beulich]

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.

> 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.

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.

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.)

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'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).

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.

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.

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.

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

@@ -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).

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.

Thanks, Jan