RE: [PATCH 2/8] Support APX GPR32 with extend evex prefix

["Cui, Lili" <lili.cui@intel.com>]

> > --- a/opcodes/i386-dis-evex-mod.h
> > +++ b/opcodes/i386-dis-evex-mod.h
> > @@ -1 +1,51 @@
> >  /* Nothing at present.  */
> > +  /* MOD_EVEX_MAP4_65 */
> > +  {
> > +    { "wrussK",		{ M, Gdq }, PREFIX_DATA },
> > +  },
> > +  /* MOD_EVEX_MAP4_66_PREFIX_0 */
> > +  {
> > +    { "wrssK",		{ M, Gdq }, 0 },
> > +  },
> 
> Not very long ago I invested quite a bit of time to remove unnecessary
> decoding through mod_table[]. Please don't introduce new instances.
> Entries should be added here only when both branches are populated (iow it
> looks as if this patch shouldn't touch this file at all).
> 

Done.

> > +  /* PREFIX_EVEX_MAP4_60 */
> > +  {
> > +    { "movbeS",	{ Gv, Ev }, 0 },
> > +    { Bad_Opcode },
> > +    { "movbeS",	{ Gv, Ev }, 0 },
> > +  },
> > +  /* PREFIX_EVEX_MAP4_61 */
> > +  {
> > +    { "movbeS",	{ Ev, Gv }, 0 },
> > +    { Bad_Opcode },
> > +    { "movbeS",	{ Ev, Gv }, 0 },
> > +  },
> 
> In cases like this (of which, aiui, there will be many more), where only prefix
> 66 is valid, and only to modify operand size, it would be quite desirable to
> have a new PREFIX_... identifier to use in the parent table entry, such that this
> additional decode step can be avoided.
> 

Added PREFIX_DATA_AND_NP_ONLY to report bad for f2 and f3 prefix. Do you have a better name for the new PREFIX ?
Also added two bad.s test cases for it.

        #movbe %r18w,%ax set EVEX.pp = f3 (illegal value).
        .byte 0x62, 0xfc, 0x7e, 0x08, 0x60, 0xc2
        #movbe %r18w,%ax set EVEX.pp = f2 (illegal value).
        .byte 0x62, 0xfc, 0x7f, 0x08, 0x60, 0xc2

> > --- /dev/null
> > +++ b/opcodes/i386-dis-evex-x86.h
> 
> I'm puzzled by the name suffix: x86 is kind of redundant with i386. Main
> question perhaps is: Do we really need a new file here? It's not a lot that is
> put here right now, but of course I haven't peeked ahead.

I think it should be i386-dis-evex-x86-64.h. This table is dedicated to the VEX promotion instruction. It is placed at the end of x86_64_table and marked with ins->evex_type = evex_from_vex.
It can share partial tables with VEX after x86-64 table.

   case USE_X86_64_EVEX_FROM_VEX_TABLE:
      ins->evex_type = evex_from_vex;
      /* Fall through.  */
    case USE_X86_64_TABLE:
      vindex = ins->address_mode == mode_64bit ? 1 : 0;
      dp = &x86_64_table[dp->op[1].bytemode][vindex];
      break;

> > --- a/opcodes/i386-dis-evex.h
> > +++ b/opcodes/i386-dis-evex.h
> > @@ -164,10 +164,10 @@ static const struct dis386 evex_table[][256] = {
> >      /* F8 */
> >      { Bad_Opcode },
> >      { Bad_Opcode },
> > @@ -854,7 +854,7 @@ static const struct dis386 evex_table[][256] = {
> >      { Bad_Opcode },
> >      { Bad_Opcode },
> >      /* F0 */
> > -    { Bad_Opcode },
> > +    { X86_64_EVEX_FROM_VEX_TABLE (X86_64_EVEX_0F3AF0) },
> >      { Bad_Opcode },
> >      { Bad_Opcode },
> >      { Bad_Opcode },
> > @@ -872,7 +872,7 @@ static const struct dis386 evex_table[][256] = {
> >      { Bad_Opcode },
> >      { Bad_Opcode },
> >    },
> > -  /* EVEX_MAP5_ */
> > +  /* EVEX_MAP4_ */
> 
> While just an artifact from this, ...
> 
> > @@ -893,8 +893,8 @@ static const struct dis386 evex_table[][256] = {
> >      { Bad_Opcode },
> >      { Bad_Opcode },
> >      /* 10 */
> > -    { PREFIX_TABLE (PREFIX_EVEX_MAP5_10) },
> > -    { PREFIX_TABLE (PREFIX_EVEX_MAP5_11) },
> > +    { Bad_Opcode },
> > +    { Bad_Opcode },
> >      { Bad_Opcode },
> >      { Bad_Opcode },
> >      { Bad_Opcode },
> > @@ -907,7 +907,7 @@ static const struct dis386 evex_table[][256] = {
> >      { Bad_Opcode },
> >      { Bad_Opcode },
> >      { Bad_Opcode },
> > -    { PREFIX_TABLE (PREFIX_EVEX_MAP5_1D) },
> > +    { Bad_Opcode },
> >      { Bad_Opcode },
> >      { Bad_Opcode },
> >      /* 20 */
> 
> ... changes like these are extremely odd to read. Can you please try to split
> this patch such that initially you simply introduce an empty new sub-table, to
> avoid such anomalies (which will also affect "git blame" then, I expect)?
> 

Sure, it's a good suggestion. I had a hard time to resolve the conflict here,  done.

> > --- a/opcodes/i386-dis.c
> > +++ b/opcodes/i386-dis.c
> > @@ -132,6 +132,13 @@ enum x86_64_isa
> >    intel64
> >  };
> >
> > +enum evex_type
> > +{
> > +  evex_default = 0,
> > +  evex_from_legacy,
> > +  evex_from_vex,
> > +};
> > +
> >  struct instr_info
> >  {
> >    enum address_mode address_mode;
> > @@ -212,7 +219,6 @@ struct instr_info
> >      int ll;
> >      bool w;
> >      bool evex;
> > -    bool r;
> 
> The change to eliminate this field would certainly be nice to be separate from
> the bulk of thw APX changes here.
> 

Thanks,
Lili.