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

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

> >> OK, I'll try to add more checks, and add description for them.
> >>
> >
> > Added some bit checks in the decoder.
> >
> > --- a/opcodes/i386-dis.c
> > +++ b/opcodes/i386-dis.c
> > @@ -11533,6 +11642,13 @@ OP_E_memory (instr_info *ins, int bytemode,
> int sizeflag)
> >                 abort ();
> >               if (ins->vex.evex)
> >                 {
> > +                 /* S/G EVEX insns require EVEX.P[10] == 1 */
> > +                 if (ins->rex2 & REX_X)
> > +                   {
> > +                     oappend (ins, "(bad)");
> > +                     return true;
> > +                   }
> 
> I think I understand this, but ...
> 
> > @@ -9003,6 +9003,9 @@ get_valid_dis386 (const struct dis386 *dp,
> instr_info *ins)
> >         case 0x4:
> >           vex_table_index = EVEX_MAP4;
> >           ins->evex_type = evex_from_legacy;
> > +         /* EVEX from legacy instrucions require EVEX.P[2:0] must be 0x04.  */
> > +         if (!((*ins->codep & 0x7) == 0x04))
> > +           return  &bad_opcode;
> 
> ... what's this about? Aren't you in a switch() dealing with the exact same
> three bits?
> 

Oh, deleted it.

> > @@ -9063,6 +9066,22 @@ get_valid_dis386 (const struct dis386 *dp,
> instr_info *ins)
> >         }
> >
> >        ins->need_vex = 4;
> > +
> > +      /* EVEX from legacy requrie EVEX.P[17:16] must be 0, EVEX.P[23:21]
> must
> > +        be 0.
> > +        EVEX from evex requrie EVEX.P[17:16] must be 0. EVEX.P[23:22] must
> > +        be 0, EVEX.P[20] must be 0.  */
> > +      if (ins->evex_type == evex_from_legacy || ins->evex_type ==
> evex_from_vex)
> > +       {
> > +         if (!((*ins->codep & 0x3) == 0)
> > +             || !((*ins->codep >> 6 & 0x3) == 0)
> > +             || (ins->evex_type == evex_from_legacy
> > +                 && !((*ins->codep >> 5 & 0x1) == 0))
> > +             || (ins->evex_type == evex_from_vex
> > +                 && !ins->vex.b))
> > +           return &bad_opcode;
> 
> I guess I'm confused here: So far we don't use EVEX.P[] as notation in
> comments. Can you please use the respective field names instead? Also can
> you please improve readability by converting !(a == b) into a != b?
> 

Changed it to:

+      /* EVEX from legacy instructions requrie vex.mask_register_specifier, vex.ll
+        and vex.zeroing must be 0.
+        EVEX from evex instrucions requrie vex.mask_register_specifier and vex.ll
+        must be 0.  */
+      if (ins->evex_type == evex_from_legacy || ins->evex_type == evex_from_vex)
+       {
+         if ((*ins->codep & 0x3) != 0
+             || (*ins->codep >> 6 & 0x3) != 0
+             || (ins->evex_type == evex_from_legacy
+                 && (*ins->codep >> 5 & 0x1) != 0)
+             || (ins->evex_type == evex_from_vex
+                 && !ins->vex.b))
+           return &bad_opcode;
+       }

Lili.