Re: [PATCH 3/3] Enable Intel MOVDIRI, MOVDIR64B instructions.

["Jan Beulich" <JBeulich at suse dot com>]

>>> On 26.04.18 at 21:49, <igor.v.tsimbalist@intel.com> wrote:
>--- /dev/null
>+++ b/gas/testsuite/gas/i386/x86-64-movdir-intel.d
>@@ -0,0 +1,18 @@
>+#as:
>+#objdump: -dw -Mintel
>+#name: x86_64 MOVDIR[I,64B] insns (Intel disassembly)
>+#source: x86-64-movdir.s
>+
>+.*: +file format .*
>+
>+
>+Disassembly of section \.text:
>+
>+0+ <_start>:
>+[ 	]*[a-f0-9]+:[ 	]*48 0f 38 f9 01[ 	]*rex\.W movdiri QWORD PTR \[rcx\],rax
>+[ 	]*[a-f0-9]+:[ 	]*66 0f 38 f8 01[ 	]*movdir64b rax,\[rcx\]
>+[ 	]*[a-f0-9]+:[ 	]*67 66 0f 38 f8 01[ 	]*movdir64b eax,\[ecx]
>+[ 	]*[a-f0-9]+:[ 	]*48 0f 38 f9 01[ 	]*rex\.W movdiri QWORD PTR \[rcx\],rax

What are the REX.W doing here (also in the AT&T counterpart)?

>--- a/opcodes/i386-opc.h
>+++ b/opcodes/i386-opc.h
>@@ -231,6 +231,10 @@ enum
>   CpuWAITPKG,
>   /* CLDEMOTE instruction required */
>   CpuCLDEMOTE,
>+  /* MOVDIRI instruction support required */
>+  CpuMOVDIRI,
>+  /* MOVDIRR64B instruction required */
>+  CpuMOVDIR64B,
>   /* MMX register support required */
>   CpuRegMMX,
>   /* XMM register support required */

Considering patch context here, this clearly wasn't re-based onto the
tip of master before submitting.

>@@ -628,6 +638,7 @@ static bitfield opcode_modifiers[] =
>   BITFIELD (ToDword),
>   BITFIELD (ToQword),
>   BITFIELD (AddrPrefixOp0),
>+  BITFIELD (AddrPrefixOpReg),

I'm not convinced this new attribute is needed - I'd much rather see
AddrPrefixOp0 re-purposed/generalized (and suitably renamed, e.g.
simply the 0 stripped). This is based on the fact that so far that
existing attribute is used only in templates not allowing for a memory
operand at all.

H.J., considering the effort I'm putting in to overcome some of the
easiest-possible-solution things that have been allowed in, I'd much
appreciate if you would push for at least investigation of whether a
more elaborate approach exists before allowing in in particular single
use new insn attributes.

>+movdiri, 2, 0xf38f9, None, 3, CpuMOVDIRI, Modrm|CheckRegSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { Reg32, Dword|Qword|Unspecified|BaseIndex|Disp8|Disp32|Disp32S }
>+movdiri, 2, 0xf38f9, None, 3, CpuMOVDIRI|Cpu64, Modrm|CheckRegSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf|Rex64, { Reg64, Dword|Qword|Unspecified|BaseIndex|Disp8|Disp32|Disp32S }

I don't understand why you need two templates here. And if you really
do, Dword on the 64-bit one looks wrong as do Qword and Disp32S on
the 32-bit one. Disp16 is missing in any case. I also don't see the point
of CheckRegSize - there is only a single register operand (the attribute
has meaning for memory operands only on SIMD templates).

>+movdir64b, 2, 0x660f38f8, None, 3, CpuMOVDIR64B|CpuNo64, Modrm|IgnoreSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf|AddrPrefixOpReg, { Unspecified|ZMMword|BaseIndex|Disp8|Disp32|Disp32S, Reg16|Reg32 }
>+movdir64b, 2, 0x660f38f8, None, 3, CpuMOVDIR64B|Cpu64, Modrm|IgnoreSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf|NoRex64|AddrPrefixOpReg, { Unspecified|ZMMword|BaseIndex|Disp8|Disp32|Disp32S, Reg32|Reg64 }

Almost all the same here. Additionally, while I can see that ZMMword fits
the 64-byte operand size, I really think it would look rather odd to have
"zmmword ptr" used on an operand here. Simply require no operand size
prefix (in Intel syntax mode), just like you make the disassembler not
produce any?

Jan