RE: [PATCH V2] Support APX NF

public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed

From: "Cui, Lili" <lili.cui@intel.com>
To: "Beulich, Jan" <JBeulich@suse.com>
Cc: "hjl.tools@gmail.com" <hjl.tools@gmail.com>,
	"binutils@sourceware.org" <binutils@sourceware.org>
Subject: RE: [PATCH V2] Support APX NF
Date: Mon, 18 Mar 2024 13:43:57 +0000	[thread overview]
Message-ID: <PH0PR11MB55932B2A0B3FA48051E684069E2D2@PH0PR11MB5593.namprd11.prod.outlook.com> (raw)
In-Reply-To: <947a7263-8075-41ed-a842-d620762d3fbe@suse.com>

> On 18.03.2024 12:21, Cui, Lili wrote:
> >> On 13.03.2024 03:54, Cui, Lili wrote:
> >>>> On 12.03.2024 14:22, Cui, Lili wrote:
> >>>>>  // Arithmetic.
> >>>>>  add, 0x0, APX_F,
> >>>>>
> D|C|W|CheckOperandSize|Modrm|No_sSuf|DstVVVV|EVexMap4|NF, {
> >>>>> Reg8|Reg16|Reg32|Reg64,
> >>>> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex,
> >>>>> Reg8|Reg16|Reg32|Reg64 } -add, 0x0, 0,
> >>>>> D|W|CheckOperandSize|Modrm|No_sSuf|HLEPrefixLock, {
> >>>>> Reg8|Reg16|Reg32|Reg64,
> >>>> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> >>>>> -add, 0x0, APX_F,
> >>>> D|W|CheckOperandSize|Modrm|No_sSuf|EVexMap4|NF, {
> >>>>> Reg8|Reg16|Reg32|Reg64,
> >>>> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> >>>>> +add<cpu1>, 0x0, 0,
> >>>> D|W|CheckOperandSize|Modrm|No_sSuf|<cpu1:attr>, {
> >>>>> +Reg8|Reg16|Reg32|Reg64,
> >>>> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex
> >>>>> +}
> >>>>
> >>>> Differentiating on other than the mnemonic is possible (see e.g.
> >>>> the mmx and sse templates, which append nothing to the mnemonics)
> >>>> but not wanted here. The goal of the suggested template is to have
> >>>> something that covers all 7 mnemonics in one go, including their legacy
> forms.
> >>>>
> >>>
> >>> The handling of <mmx/<sse are clever. The current templates are
> >>> indeed
> >> redundant. Do you mind I create a separate patch to clean up all APX
> >> templates instead of in this patch.
> >>
> >> Well - it explicitly ought to be a separate patch. Yet FTAOD - afaict
> >> such a new (set of) template(s) should not follow mmx/sse, but rather
> >> e.g. fma or pseudopfx (covering the entire mnemonic then).
> >>
> >
> > Hi Jan,
> >
> > Below are two examples of merging, does it meet expectations?
> >
> > --- a/opcodes/i386-gen.c
> > +++ b/opcodes/i386-gen.c
> >
> > @@ -1904,11 +1904,15 @@ process_i386_opcodes (FILE *table)
> >         case '<':
> >           parse_template (p, lineno);
> >           continue;
> > +       case '@':
> > +         p++;
> > +         break;
> >         default:
> >           if (!marker)
> >             continue;
> >           break;
> >         }
> >
> > rol and ror can be merged.
> >
> > -rol, 0xd0/0, APX_F,
> > W|Modrm|No_sSuf|CheckOperandSize|DstVVVV|EVexMap4|NF, { Imm1,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex,
> Reg8|Reg16|Reg32|Reg64 }
> > -rol, 0xd0/0, 0, W|Modrm|No_sSuf, { Imm1,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } -rol, 0xd0/0, APX_F,
> > W|Modrm|No_sSuf|EVexMap4|NF, { Imm1,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } -rol, 0xc0/0, APX_F,
> > W|Modrm|No_sSuf|CheckOperandSize|DstVVVV|EVexMap4|NF,
> { Imm8|Imm8S,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex,
> Reg8|Reg16|Reg32|Reg64 }
> > -rol, 0xc0/0, i186, W|Modrm|No_sSuf, { Imm8|Imm8S,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } -rol, 0xc0/0, APX_F,
> > W|Modrm|No_sSuf|EVexMap4|NF, { Imm8|Imm8S,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } -rol, 0xd2/0, APX_F,
> > W|Modrm|No_sSuf|CheckOperandSize|DstVVVV|EVexMap4|NF,
> { ShiftCount,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex,
> Reg8|Reg16|Reg32|Reg64 }
> > -rol, 0xd2/0, 0, W|Modrm|No_sSuf, { ShiftCount,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } -rol, 0xd2/0, APX_F,
> > W|Modrm|No_sSuf|EVexMap4|NF, { ShiftCount,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } -rol, 0xd0/0, 0,
> > W|Modrm|No_sSuf, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> > -rol, 0xd0/0, APX_F, W|Modrm|No_sSuf|EVexMap4|NF, {
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> > -
> > -ror, 0xd0/1, APX_F,
> > W|Modrm|No_sSuf|CheckOperandSize|DstVVVV|EVexMap4|NF, { Imm1,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex,
> Reg8|Reg16|Reg32|Reg64 }
> > -ror, 0xd0/1, 0, W|Modrm|No_sSuf, { Imm1,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } -ror, 0xd0/1, APX_F,
> > W|Modrm|No_sSuf|EVexMap4|NF, { Imm1,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } -ror, 0xc0/1, APX_F,
> > W|Modrm|No_sSuf|CheckOperandSize|DstVVVV|EVexMap4|NF,
> { Imm8|Imm8S,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex,
> Reg8|Reg16|Reg32|Reg64 }
> > -ror, 0xc0/1, i186, W|Modrm|No_sSuf, { Imm8|Imm8S,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } -ror, 0xc0/1, APX_F,
> > W|Modrm|No_sSuf|EVexMap4|NF, { Imm8|Imm8S,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } -ror, 0xd2/1, APX_F,
> > W|Modrm|No_sSuf|CheckOperandSize|DstVVVV|EVexMap4|NF,
> { ShiftCount,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex,
> Reg8|Reg16|Reg32|Reg64 }
> > -ror, 0xd2/1, 0, W|Modrm|No_sSuf, { ShiftCount,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } -ror, 0xd2/1, APX_F,
> > W|Modrm|No_sSuf|EVexMap4|NF, { ShiftCount,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } -ror, 0xd0/1, 0,
> > W|Modrm|No_sSuf, { Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> > -ror, 0xd0/1, APX_F, W|Modrm|No_sSuf|EVexMap4|NF, {
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> > +<extend10:cpu:attr:reg, +
> > +
> $ndd:APX_F:CheckOperandSize|DstVVVV|EVexMap4|NF:Reg8|Reg16|Reg3
> 2|Reg64, +
> > +       $legacy:0::, +
> > +       $nf:APX_F:EVexMap4|NF:>
> > +
> > +<extend11:cpu:attr:reg, +
> > +
> $ndd:APX_F:CheckOperandSize|DstVVVV|EVexMap4|NF:Reg8|Reg16|Reg3
> 2|Reg64, +
> > +       $legacy:i186::, +
> > +       $nf:APX_F:EVexMap4|NF:>
> > +
> > +<extend12:cpu:attr, +
> > +       $legacy:0:, +
> > +       $nf:APX_F:EVexMap4|NF>
> 
> I don't understand these three. I'd have expected all attributes to be
> specified ...
> 
> > +<mn:opcode, rol:0, ror:1>
> 
> ... here. Same then for add, where I'd have expected a similar template as the
> one above, e.g.
> 
> <alu:opcode, add:0, sub:...>
> 
> (but of course with more than just "opcode" specified).
> 
> > +@<mn><extend10>, 0xd0/0|<mn:opcode>, <extend10:cpu>,
> > +W|Modrm|No_sSuf|<extend10:attr>, { Imm1,
> > +Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex, <extend10:reg> }
> > +
> > +@<mn><extend11>, 0xc0/0|<mn:opcode>, <extend11:cpu>,
> > +W|Modrm|No_sSuf|<extend11:attr>, { Imm8|Imm8S,
> > +Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex, <extend11:reg> }
> > +
> > +@<mn><extend10>, 0xd2/0|<mn:opcode>, <extend10:cpu>,
> > +W|Modrm|No_sSuf|<extend10:attr>, { ShiftCount,
> > +Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex, <extend10:reg> }
> > +
> > +@<mn><extend12>, 0xd0/0|<mn:opcode>, <extend12:cpu>,
> > +W|Modrm|No_sSuf|<extend12:attr>, {
> > +Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> > +
> > ...
> >
> > The same instruction as add structure was not found. There are always some
> subtle differences.
> 
> I expect the issue is with you using just a single "attr", trying to put all
> attributes in there. When split (see e.g. <sdh>), folding ought to be possible.
> 

Oh, get you. It can merge more templates.

> I'd also prefer if we could avoid the need for the leading @.
> 

Since they all start with '<', i386-gen.c needs to use '<' to distinguish them, we need to add something before the second '<' ,  so I added @ as a temporary workaround. We really need a more reasonable logo.

<mn:opcode, rol:0, ror:1>
<mn><extend10>, 0xd0/0|<mn:opcode>, <extend10:cpu>, W|Modrm|No_sSuf|<extend10:attr>, { Imm1, Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex, <extend10:reg> }

> Anyway - feel free to leave this to me. While not overly nice, I guess we can live
> with the growing volume due to what NF wants to add.
> 
> Jan

Ok, thanks.

Lili.
> 
> > +<extend1:cpu:attr:reg, +
> > +       $ndd:APX_F:C|DstVVVV|EVexMap4|NF:Reg8|Reg16|Reg32|Reg64, +
> > +       $legacy:0:HLEPrefixLock:, +
> > +       $nf:APX_F:EVexMap4|NF:>
> > +
> > +<extend2:cpu:attr:reg, +
> > +
> $ndd:APX_F:CheckOperandSize|DstVVVV|EVexMap4|NF:Reg16|Reg32|Reg
> 64, +
> > +       $legacy:0:HLEPrefixLock:, +
> > +       $nf:APX_F:EVexMap4|NF:>
> > +
> > +<extend3:cpu:attr:reg, +
> > +
> $ndd:APX_F:CheckOperandSize|DstVVVV|EVexMap4|NF:Reg8|Reg16|Reg3
> 2|Reg64, +
> > +       $legacy:0:HLEPrefixLock:, +
> > +       $nf:APX_F:EVexMap4|NF:>
> > +
> > -add, 0x0, APX_F,
> > D|C|W|CheckOperandSize|Modrm|No_sSuf|DstVVVV|EVexMap4|NF, {
> > Reg8|Reg16|Reg32|Reg64,
> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex,
> > Reg8|Reg16|Reg32|Reg64 } -add, 0x0, 0,
> > D|W|CheckOperandSize|Modrm|No_sSuf|HLEPrefixLock, {
> > Reg8|Reg16|Reg32|Reg64,
> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> > -add, 0x0, APX_F,
> D|W|CheckOperandSize|Modrm|No_sSuf|EVexMap4|NF, {
> > Reg8|Reg16|Reg32|Reg64,
> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> > -add, 0x83/0, APX_F,
> > Modrm|CheckOperandSize|No_bSuf|No_sSuf|DstVVVV|EVexMap4|NF,
> { Imm8S,
> > Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 } -add,
> > 0x83/0, 0, Modrm|No_bSuf|No_sSuf|HLEPrefixLock, { Imm8S,
> > Reg16|Reg32|Reg64|Unspecified|BaseIndex } -add, 0x83/0, APX_F,
> > Modrm|No_bSuf|No_sSuf|EVexMap4|NF, { Imm8S,
> > Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> > +add<extend1>, 0x0, <extend1:cpu>,
> > +D|W|CheckOperandSize|Modrm|No_sSuf|<extend1:attr>, {
> > +Reg8|Reg16|Reg32|Reg64,
> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex,
> > +<extend1:reg> } add<extend2>, 0x83/0, <extend2:cpu>,
> > +Modrm|No_bSuf|No_sSuf|<extend2:attr>, { Imm8S,
> > +Reg16|Reg32|Reg64|Unspecified|BaseIndex, <extend2:reg> }
> >  add, 0x4, 0, W|No_sSuf, { Imm8|Imm16|Imm32|Imm32S,
> > Acc|Byte|Word|Dword|Qword } -add, 0x80/0, APX_F,
> > W|Modrm|CheckOperandSize|No_sSuf|DstVVVV|EVexMap4|NF, {
> > Imm8|Imm16|Imm32|Imm32S,
> Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex,
> > Reg8|Reg16|Reg32|Reg64} -add, 0x80/0, 0,
> > W|Modrm|No_sSuf|HLEPrefixLock, { Imm8|Imm16|Imm32|Imm32S,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex } -add, 0x80/0, APX_F,
> > W|Modrm|No_sSuf|EVexMap4|NF, { Imm8|Imm16|Imm32|Imm32S,
> > Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex }
> > +add<extend3>, 0x80/0, <extend3:cpu>,
> W|Modrm|No_sSuf|<extend3:attr>,
> > +{ Imm8|Imm16|Imm32|Imm32S,
> > +Reg8|Reg16|Reg32|Reg64|Unspecified|BaseIndex, <extend3:reg> }
> >
> >
> > Thanks,
> > Lili.

next prev parent reply	other threads:[~2024-03-18 13:44 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-04  8:15 Cui, Lili
2024-03-08  9:36 ` Jan Beulich
2024-03-11 13:54   ` Cui, Lili
2024-03-11 14:09     ` Jan Beulich
2024-03-12  6:12       ` Cui, Lili
2024-03-12  7:46         ` Jan Beulich
2024-03-12  8:51           ` Cui, Lili
2024-03-12 13:22   ` Cui, Lili
2024-03-12 13:53     ` Jan Beulich
2024-03-13  2:54       ` Cui, Lili
2024-03-13  7:36         ` Jan Beulich
2024-03-18 11:21           ` Cui, Lili
2024-03-18 11:50             ` Jan Beulich
2024-03-18 13:43               ` Cui, Lili [this message]
2024-03-19  1:24         ` Cui, Lili
2024-03-08 10:40 ` Jan Beulich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=PH0PR11MB55932B2A0B3FA48051E684069E2D2@PH0PR11MB5593.namprd11.prod.outlook.com \
    --to=lili.cui@intel.com \
    --cc=JBeulich@suse.com \
    --cc=binutils@sourceware.org \
    --cc=hjl.tools@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Be sure your reply has a Subject: header at the top and a blank line before the message body.

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).