[PATCH v2] Support Intel AMX-AVX512
Jan Beulich
jbeulich@suse.com
Mon Jan 6 16:29:15 GMT 2025
On 03.01.2025 03:50, Haochen Jiang wrote:
> Hi all,
>
> Although there is still opensin AMX-AVX512 encoding, I would like
> to first send out the v2 patch with the current encodings since in
> this patch, there are also other parts need to be reviewed.
>
> Patch descrption and changes are embedded following.
>
> The encoding issue I mentioned previously is on tcvtrowps2[bf16,ph][h,l].
> For Reg32 part, it is ok. However, for Imm8 part, under current HW design,
> it is split to two opcodes. It is not an ideal design. Due to Christmas/
> New Year Holiday, the answer for whether it could be changed is still
> delayed for now. I will update that as soon as I get the answer.
>
> Thx,
> Haochen
>
> ---
>
> Changes in v2:
>
> - Pull out all GPR mode out of vex length switch in OP_VEX to make
> it more general.
> - Remove invalid test for 32-bit.
> - Reuse VexGdq for operands.
> - Update the mnemonics from tcvtrowps2pbf16[h,l] to tcvtrowps2bf16[h,l]
> according to ISE056.
>
> ---
>
> This patch will support AMX-AVX512. In disassmbler, we pull out all
> GPR mode out of the vex length switch to make it more general.
>
> ---
>
> gas/ChangeLog:
>
> * config/tc-i386.c: Add amx_avx512.
> * doc/c-i386.texi: Document .amx_avx512.
> * testsuite/gas/i386/x86-64.exp: Run AMX-AVX512 tests.
> * testsuite/gas/i386/x86-64-amx-avx512-intel.d: New test.
> * testsuite/gas/i386/x86-64-amx-avx512.d: Ditto.
> * testsuite/gas/i386/x86-64-amx-avx512.s: Ditto.
>
> opcodes/ChangeLog:
>
> * i386-dis-evex-len.h: Add EVEX_LEN_0F384A_X86_64_W_0,
> EVEX_LEN_0F386D_X86_64_W_0, EVEX_LEN_0F3A07_X86_64_W_0,
> EVEX_LEN_0F3A77_X86_64_W_0.
> * i386-dis-evex-prefix.h: Add PREFIX_EVEX_0F384A_W_0_L_2,
> PREFIX_EVEX_0F386D_W_0_L_2, PREFIX_EVEX_0F3A07_W_0_L_2,
> PREFIX_EVEX_0F3A77_W_0_L_2.
> * i386-dis-evex-w.h: Add EVEX_W_0F384A_X86_64, EVEX_W_0F386D_X86_64,
> EVEX_W_0F3A07_X86_64, EVEX_W_0F3A77_X86_64.
> * i386-dis-evex-x86-64.h: Add X86_64_EVEX_0F384A, X86_64_EVEX_0F386D,
> X86_64_EVEX_0F3A07, X86_64_EVEX_0F3A77.
> * i386-dis-evex.h: Ditto.
> * i386-dis.c (EVEX_LEN_0F384A_X86_64_W_0): New.
> (EVEX_LEN_0F386D_X86_64_W_0): Ditto.
> (EVEX_LEN_0F3A07_X86_64_W_0): Ditto.
> (EVEX_LEN_0F3A77_X86_64_W_0): Ditto.
> (MOD_EVEX_0F384A_X86_64_W_0): Ditto.
> (MOD_EVEX_0F386D_X86_64_W_0): Ditto.
> (MOD_EVEX_0F3A07_X86_64_W_0): Ditto.
> (MOD_EVEX_0F3A77_X86_64_W_0): Ditto.
> (PREFIX_EVEX_0F384A_W_0_L_2): Ditto.
> (PREFIX_EVEX_0F386D_W_0_L_2): Ditto.
> (PREFIX_EVEX_0F3A07_W_0_L_2): Ditto.
> (PREFIX_EVEX_0F3A77_W_0_L_2): Ditto.
> (EVEX_W_0F384A_X86_64): Ditto.
> (EVEX_W_0F386D_X86_64): Ditto.
> (EVEX_W_0F3A07_X86_64): Ditto.
> (EVEX_W_0F3A77_X86_64): Ditto.
> (X86_64_EVEX_0F384A): Ditto.
> (X86_64_EVEX_0F386D): Ditto.
> (X86_64_EVEX_0F3A07): Ditto.
> (X86_64_EVEX_0F3A77): Ditto.
> (OP_VEX): Pull out all GPR mode out of the vector length switch.
> * i386-gen.c (isa_dependencies): Add AMX-AVX512.
> (cpu_flags): Ditto.
> * i386-init.h: Regenerated.
> * i386-mnem.h: Ditto.
> * i386-opc.h (CpuAMX_AVX512): New.
> (i386_cpu_flags): Add cpuamx_avx512.
> * i386-opc.tbl: Add AMX-AVX512 instructions.
> * i386-tbl.h: Regenerated.
> ---
> gas/config/tc-i386.c | 1 +
> gas/doc/c-i386.texi | 4 +-
> .../gas/i386/x86-64-amx-avx512-intel.d | 35 +
> gas/testsuite/gas/i386/x86-64-amx-avx512.d | 34 +
> gas/testsuite/gas/i386/x86-64-amx-avx512.s | 55 +
> gas/testsuite/gas/i386/x86-64.exp | 2 +
> opcodes/i386-dis-evex-len.h | 23 +
> opcodes/i386-dis-evex-prefix.h | 27 +
> opcodes/i386-dis-evex-w.h | 12 +
> opcodes/i386-dis-evex-x86-64.h | 15 +
> opcodes/i386-dis-evex.h | 6 +-
> opcodes/i386-dis.c | 52 +-
> opcodes/i386-gen.c | 3 +
> opcodes/i386-init.h | 718 ++---
> opcodes/i386-mnem.h | 2576 +++++++++--------
> opcodes/i386-opc.h | 3 +
> opcodes/i386-opc.tbl | 15 +
> opcodes/i386-tbl.h | 415 ++-
> 18 files changed, 2210 insertions(+), 1786 deletions(-)
> create mode 100644 gas/testsuite/gas/i386/x86-64-amx-avx512-intel.d
> create mode 100644 gas/testsuite/gas/i386/x86-64-amx-avx512.d
> create mode 100644 gas/testsuite/gas/i386/x86-64-amx-avx512.s
It is again nowhere in the patch metadata that you put down what other
non-upstream patch(es) this one goes on top of. This is important info
for a reviewer. Since this isn't the first time, let me make it quite
clear: Going forward I may simply refuse to review (reject) patches
with unclear dependencies.
> @@ -14070,6 +14083,29 @@ OP_VEX (instr_info *ins, int bytemode, int sizeflag ATTRIBUTE_UNUSED)
> return true;
> }
>
> + switch (bytemode)
> + {
> + case v_mode:
> + case dq_mode:
> + if (ins->rex & REX_W)
> + names = att_names64;
> + else if (bytemode == v_mode
> + && !(sizeflag & DFLAG))
> + names = att_names16;
> + else
> + names = att_names32;
> + oappend_register (ins, names[reg]);
> + return true;
> + case b_mode:
> + names = att_names8rex;
> + oappend_register (ins, names[reg]);
> + return true;
> + case q_mode:
> + names = att_names64;
> + oappend_register (ins, names[reg]);
> + return true;
> + }
I think there are two ways to improve legibility here: Either pull out the
call to oappend_register() (and the return), or avoid using the "names"
local var in the latter two cases.
> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -3243,6 +3243,21 @@ t2rpntlvw<z>rs<loc>, 0x<z:pfx>f8 | <loc:opc>, AMX_TRANSPOSE&APX_F(AMX_MOVRS), Si
> tileloaddrs, 0xf24a, APX_F(AMX_MOVRS), Sibmem|Vex128|EVex128|Space0F38|VexW0|NoSuf, { Unspecified|BaseIndex, RegTMM }
> tileloaddrst1, 0x664a, APX_F(AMX_MOVRS), Sibmem|Vex128|EVex128|Space0F38|VexW0|NoSuf, { Unspecified|BaseIndex, RegTMM }
>
> +tcvtrowd2ps, 0xf34a, AMX_AVX512, Modrm|EVex512|Space0F38|Src2VVVV|VexW0|NoSuf, { Reg32, RegTMM, RegZMM }
> +tcvtrowd2ps, 0xf307, AMX_AVX512, Modrm|EVex512|Space0F3A|VexW0|NoSuf, { Imm8, RegTMM, RegZMM }
> +
> +tcvtrowps2bf16h, 0xf26d, AMX_AVX512, Modrm|EVex512|Space0F38|Src2VVVV|VexW0|NoSuf, { Reg32, RegTMM, RegZMM }
> +tcvtrowps2bf16h, 0xf207, AMX_AVX512, Modrm|EVex512|Space0F3A|VexW0|NoSuf, { Imm8, RegTMM, RegZMM }
> +tcvtrowps2bf16l, 0xf36d, AMX_AVX512, Modrm|EVex512|Space0F38|Src2VVVV|VexW0|NoSuf, { Reg32, RegTMM, RegZMM }
> +tcvtrowps2bf16l, 0xf377, AMX_AVX512, Modrm|EVex512|Space0F3A|VexW0|NoSuf, { Imm8, RegTMM, RegZMM }
> +tcvtrowps2phh, 0x6d, AMX_AVX512, Modrm|EVex512|Space0F38|Src2VVVV|VexW0|NoSuf, { Reg32, RegTMM, RegZMM }
> +tcvtrowps2phh, 0x07, AMX_AVX512, Modrm|EVex512|Space0F3A|VexW0|NoSuf, { Imm8, RegTMM, RegZMM }
> +tcvtrowps2phl, 0x666d, AMX_AVX512, Modrm|EVex512|Space0F38|Src2VVVV|VexW0|NoSuf, { Reg32, RegTMM, RegZMM }
> +tcvtrowps2phl, 0xf277, AMX_AVX512, Modrm|EVex512|Space0F3A|VexW0|NoSuf, { Imm8, RegTMM, RegZMM }
> +
> +tilemovrow, 0x664a, AMX_AVX512, Modrm|EVex512|Space0F38|Src2VVVV|VexW0|NoSuf, { Reg32, RegTMM, RegZMM }
> +tilemovrow, 0x6607, AMX_AVX512, Modrm|EVex512|Space0F3A|VexW0|NoSuf, { Imm8, RegTMM, RegZMM }
Just to double check: The AVX10.2/256 case really is of no interest for this
feature / these insns, and your designers would rather introduce yet another
CPUID flag in case it turned out desirable later on?
Jan
More information about the Binutils
mailing list