Re: [PATCH]

[Jan Beulich <jbeulich@suse.com>]

On 31.10.2022 05:37, Joshi, Tejas Sanjay wrote:
>> You also (afaict needlessly) add CPU_RMPQUERY_FLAGS, whereas 
>> CpuRMPQUERY isn't applicable here; you already ...
>>
>>>       (cpu_flags): Add CpuRMPQUERY.
>>
>> ... mention it here, where it is applicable.
> 
> Corrected this in the ChangeLog.
> 
>>>       * i386-opc.h (enum): Add CpuRMPQUERY.
>>>       (i386_cpu_flags): Add cpurmpquery.
>>>       * i386-opc.tbl: Add rmpquery insn.
>>
>> The three relevant SNP insn have a comment: "The single-operand forms 
>> exist only for compatibility with older gas." Therefore I'd like to 
>> ask that new insns (like the one you add here) don't have that strange 
>> form also added.
> 
> Yes, the single-operand form for rmpquery is also added in this patch (RMPQUERY 
> has virtual address specified in RAX register):
> 
> +rmpquery, 0xf30f01fd, None, CpuRMPQUERY|Cpu64, AddrPrefixOpReg, { Acc|Dword|Qword }

That's what the 3-operand form is intended for. For insns like this we
mean to allow a shorthand (here: no operands) and a variant with _all_
operands spelled out. There's no need for "half" a shorthand. See also
H.J.'s response. And note how you already don't test the 1-operand form.

> opcodes/
> 
> 	* i386-dis.c (x86_64_table): Add rmpquery.
> 	* i386-gen.c (cpu_flag_init): Add CPU_ZNVER4_FLAGS and
> 	CPU_RMPQUERY_FLAGS.

I see you's corrected the changelog entry, but as previously pointed
out it's unclear what you add CPU_RMPQUERY_FLAGS for. You don't use it
anywhere.

> --- /dev/null
> +++ b/gas/testsuite/gas/i386/arch-14-1.d
> @@ -0,0 +1,22 @@
> +#objdump: -dw
> +#name: i386 arch 14-1
> +
> +.*:     file format .*
> +
> +Disassembly of section .text:
> +
> +0+ <.text>:
> +[ 	]*[a-f0-9]+:	62 f1 d5 48 58 f4    	vaddpd %zmm4,%zmm5,%zmm6
> +[ 	]*[a-f0-9]+:[ 	]*62 f2 7d 48 1b 31[ 	]*vbroadcastf32x8 \(%ecx\),%zmm6
> +[ 	]*[a-f0-9]+:[ 	]*62 f2 d5 48 b4 f4[ 	]*vpmadd52luq %zmm4,%zmm5,%zmm6
> +[ 	]*[a-f0-9]+:	62 f2 7d 48 c4 f5    	vpconflictd %zmm5,%zmm6
> +[ 	]*[a-f0-9]+:[ 	]*62 f2 7d 48 1c f5[ 	]*vpabsb %zmm5,%zmm6
> +[ 	]*[a-f0-9]+:[ 	]*62 f1 d5 0f 58 f4[ 	]*vaddpd %xmm4,%xmm5,%xmm6\{%k7\}
> +[ 	]*[a-f0-9]+:	62 f2 57 48 72 f4    	vcvtne2ps2bf16 %zmm4,%zmm5,%zmm6
> +[ 	]*[a-f0-9]+:[ 	]*62 f2 55 48 8d f4[ 	]*vpermb %zmm4,%zmm5,%zmm6
> +[ 	]*[a-f0-9]+:[ 	]*62 f2 7d 4f 63 31[ 	]*vpcompressb %zmm6,\(%ecx\)\{%k7\}
> +[ 	]*[a-f0-9]+:[ 	]*62 f2 75 48 52 e3[ 	]*vpdpwssd %zmm3,%zmm1,%zmm4
> +[ 	]*[a-f0-9]+:[ 	]*62 f2 55 48 8f ec[ 	]*vpshufbitqmb %zmm4,%zmm5,%k5
> +[ 	]*[a-f0-9]+:[ 	]*62 f2 7d 48 55 f5[ 	]*vpopcntd %zmm5,%zmm6
> +[ 	]*[a-f0-9]+:[ 	]*66 0f 38 cf ec[ 	]*gf2p8mulb %xmm4,%xmm5

Please be consistent with output expectations at the very least within an
individual new test: You want to use [ 	]* also on the three lines where
so far you don't. Same for the 64-bit counterpart then of course, albeit
...

> --- /dev/null
> +++ b/gas/testsuite/gas/i386/x86-64-arch-4-1.d
> @@ -0,0 +1,23 @@
> +#objdump: -dw
> +#name: x86-64 arch 4-1
> +
> +.*:     file format .*
> +
> +Disassembly of section .text:
> +
> +0+ <.text>:
> +[ 	]*[a-f0-9]+:	62 01 95 40 58 f4    	vaddpd %zmm28,%zmm29,%zmm30
> +[ 	]*[a-f0-9]+:[ 	]*62 62 7d 48 1b 31[ 	]*vbroadcastf32x8 \(%rcx\),%zmm30
> +[ 	]*[a-f0-9]+:[ 	]*62 02 95 40 b4 f4[ 	]*vpmadd52luq %zmm28,%zmm29,%zmm30
> +[ 	]*[a-f0-9]+:	62 02 7d 48 c4 f5    	vpconflictd %zmm29,%zmm30
> +[ 	]*[a-f0-9]+:[ 	]*62 02 7d 48 1c f5[ 	]*vpabsb %zmm29,%zmm30
> +[ 	]*[a-f0-9]+:[ 	]*62 01 95 00 58 f4[ 	]*vaddpd %xmm28,%xmm29,%xmm30
> +[ 	]*[a-f0-9]+:	62 02 17 40 72 f4    	vcvtne2ps2bf16 %zmm28,%zmm29,%zmm30
> +[ 	]*[a-f0-9]+:[ 	]*62 02 15 40 8d f4[ 	]*vpermb %zmm28,%zmm29,%zmm30
> +[ 	]*[a-f0-9]+:[ 	]*62 62 7d 4f 63 31[ 	]*vpcompressb %zmm30,\(%rcx\)\{%k7\}
> +[ 	]*[a-f0-9]+:[ 	]*62 a2 6d 40 52 d1[ 	]*vpdpwssd %zmm17,%zmm18,%zmm18
> +[ 	]*[a-f0-9]+:[ 	]*62 92 15 40 8f ec[ 	]*vpshufbitqmb %zmm28,%zmm29,%k5
> +[ 	]*[a-f0-9]+:[ 	]*62 02 7d 48 55 f5[ 	]*vpopcntd %zmm29,%zmm30
> +[ 	]*[a-f0-9]+:[ 	]*66 0f 38 cf ec[ 	]*gf2p8mulb %xmm4,%xmm5
> +[ 	]*[a-f0-9]+:[ 	]*f3 0f 01 fd[ 	]*rmpquery[ 	]*

... this last line shouldn't need the trailing [ 	]* (iirc as of recently
it doesn't even need " *" anymore).

Jan