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

["Tsimbalist, Igor V" <igor dot v dot tsimbalist at intel dot com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, April 27, 2018 8:41 AM
> To: H.J. Lu <hjl.tools@gmail.com>; Tsimbalist, Igor V
> <igor.v.tsimbalist@intel.com>
> Cc: binutils@sourceware.org
> Subject: Re: [PATCH 3/3] Enable Intel MOVDIRI, MOVDIR64B instructions.
> 
> >>> 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)?

The spec definitely says REX.W has to be used to promote operation to 64bit.

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

The patch will be reverted.
 
> >@@ -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.

Will redo this part as well as parts below.

> 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|R
> ex64, { 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|Addr
> PrefixOpReg, { 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|NoR
> ex64|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?

Yes, that was one of my concerns also but I decided to keep it there as spec
mentions m512. So asm will not accept and objdump will not print 'zmmword ptr'.

Igor


> Jan
>