[PATCH v2] Support Intel AVX10.2 media instructions

[Jan Beulich]

On 09.09.2024 10:18, Haochen Jiang wrote:
> Nit: I will rebase to the following Jan's patch after that patch's
> AVX-VNNI-INT8/16 being implied by AVX10.2 part committed to trunk.
> 
> https://sourceware.org/pipermail/binutils/2024-September/136630.html

I'm puzzled - didn't you ask that we not do this move?

> ---
> For vnni instructions, we extended previous VEX part using %XE
> in disassembler to promote them to EVEX.

Yet this doesn't look to be exercised anywhere in the testcases.

> We could not use this
> method to vmpsadbw due to different prefix although it is a
> promotion from AVX2.

I'm afraid I don't understand this. Why can't you use %XE in the new
table entry? (Of course you can't simply re-use the original one.)

> For assmbler part, we put the vnni table
> entries with previous vnni instructions since most of them

Right, that's why I made the templates for them to (also) use.

Related to this placement: Did you notice the resulting anomaly in
encodings used? Without resorting to knowledge of the internal
workings of gas, what would you predict this

	vpdpbssd	%xmm1, %xmm2, %xmm3
	vpdpbusd	%xmm1, %xmm2, %xmm3
	vpdpwssd	%xmm1, %xmm2, %xmm3
	vpdpwusd	%xmm1, %xmm2, %xmm3

assembles to (VEX vs EVEX) with your patch in place? And then the
same with e.g.

	.arch generic64
	.arch .avx10.2/256

in place ahead of the block. The patch of mine you reference above
is related to this, but it doesn't fully resolve the anomaly. I'm
still trying to think of a good solution. My expectation certainly
would be that the latest with the AVX10.2 forms added, the VEX
forms would be used by default everywhere, as that's the usual
EVEX-postdates-VEX situation. Yet part of that would be that
AVX-VNNI{,-INT8,-INT16} were formal prereqs of AVX10.2 (hence in
turn that earlier patch, as a first step in that direction).

> (expect for vdpphps, which is new instruction but still vnni,
> and vmpsadbw is a promotion from AVX2)

Sure

> are just promotion from AVX-VNNI-INT{8,16}.
>[...]
> --- /dev/null
> +++ b/gas/testsuite/gas/i386/avx10_2-256-1-intel.d
> @@ -0,0 +1,151 @@
> +#objdump: -dw -Mintel
> +#name: i386 AVX10.2/256 media insns (Intel disassembly)
> +#source: avx10_2-256-1.s
> +
> +.*: +file format .*
> +
> +Disassembly of section \.text:
> +
> +0+ <_start>:
> +#...
> +\s*416:\s*62 f2 57 2f 50 f4\s+vpdpbssd ymm6\{k7\},ymm5,ymm4

I guess I can see why you don't use [a-f0-9]+ here, but no, that's
not nice. How about you add a label ahead of the Intel syntax block,
and then use that as "anchor"? (Similarly applicable to other tests
then.)

> --- a/opcodes/i386-opc.tbl
> +++ b/opcodes/i386-opc.tbl
> @@ -3073,26 +3073,41 @@ vpdpwssd<vnni><sat>, 0x6652|<sat:opc>, <vnni:avx>_VNNI, Modrm|Space0F38|Src1VVVV
>  
>  // {AVX512,AVX}_VNNI instructions end
>  
> -// AVX-VNNI-INT8 instructions.
> +// AVX-VNNI-INT8/AVX10.2 instructions.
> +
> +<vnni2:avx:attr:reg:mem, $y:_VNNI_INT8:Vex::, $z:10_2:Masking|Broadcast|Disp8ShiftVL:RegZMM:Dword>
>  
>  <dpb:pfx, uu:, ss:f2, su:f3>
>  
> -vpdpb<dpb>d<sat>, 0x<dpb:pfx>50|<sat:opc>, AVX_VNNI_INT8, Modrm|Vex|Space0F38|Src1VVVV|VexW0|CheckOperandSize|NoSuf, { RegXMM|RegYMM|Unspecified|BaseIndex, RegXMM|RegYMM, RegXMM|RegYMM }
> +vpdpb<dpb>d<vnni2><sat>, 0x<dpb:pfx>50|<sat:opc>, AVX<vnni2:avx>, Modrm|Space0F38|Src1VVVV|VexW0|<vnni2:attr>|CheckOperandSize|NoSuf, { RegXMM|RegYMM|<vnni2:reg>|<vnni2:mem>|Unspecified|BaseIndex, RegXMM|RegYMM|<vnni2:reg>, RegXMM|RegYMM|<vnni2:reg> }
> +
> +<vnni2>

Hmm, no, this wasn't the plan. The plan was that you'd add a single new
line re-using the existing templatization, without adding yet further one.
(It's also hard to see what the 2 and 3 in the template names are meant
to represent.)

> -// AVX-VNNI-INT8 instructions end.
> +// AVX-VNNI-INT8/AVX10.2 instructions end.

I don't think the comments should be changed this way. I'd suggest to leave
it alone. If you absolutely want to change them, wording needs to be
accurate: This is not the end of AVX10.2 insns, and on the whole this
section also isn't holding all of them.

> +// AVX10.2 media instructions.

In turn this may want to mention that some live elsewhere. I wonder anyway
if all new ones wouldn't better go together, at which point this may not be
the best place for inserting (and "media" would then also want dropping).

Also no full stop please at the end of these comments - good examples are
even visible in patch context.

> +vdpphps, 0x52, AVX10_2, Modrm|Space0F38|Src1VVVV|Masking|VexW0|Broadcast|Disp8ShiftVL|CheckOperandSize|NoSuf, { RegXMM|RegYMM|RegZMM|Dword|Unspecified|BaseIndex, RegXMM|RegYMM|RegZMM, RegXMM|RegYMM|RegZMM }
> +vmpsadbw, 0xf342, AVX10_2, Modrm|Masking|Space0F3A|Src1VVVV|VexW0|Disp8ShiftVL|CheckOperandSize|NoSuf, { Imm8, RegXMM|RegYMM|RegZMM|Unspecified|BaseIndex, RegXMM|RegYMM|RegZMM, RegXMM|RegYMM|RegZMM }

Please can you order attributes the same at the very least in templates
sitting directly next to each other? This makes it easier to spot
similarities / differences. E.g. like this (as applicable)

Modrm|Space...|VexW0|Src1VVVV|Broadcast|Masking|Disp8ShiftVL|CheckOperandSize|NoSuf

Jan