This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
RE: [PATCH 3/3] Enable Intel MOVDIRI, MOVDIR64B instructions.
- From: "Tsimbalist, Igor V" <igor dot v dot tsimbalist at intel dot com>
- To: Jan Beulich <JBeulich at suse dot com>, "H.J. Lu" <hjl dot tools at gmail dot com>
- Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>, "Tsimbalist, Igor V" <igor dot v dot tsimbalist at intel dot com>
- Date: Fri, 27 Apr 2018 10:06:59 +0000
- Subject: RE: [PATCH 3/3] Enable Intel MOVDIRI, MOVDIR64B instructions.
- Dlp-product: dlpe-windows
- Dlp-reaction: no-action
- Dlp-version: 11.0.200.100
- References: <D511F25789BA7F4EBA64C8A63891A002AFDCBBE8@IRSMSX102.ger.corp.intel.com> <5AE2C5ED02000078001BEFD9@prv1-mh.provo.novell.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
>