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

[Cui, Lili]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, October 20, 2023 2:26 PM
> To: Cui, Lili <lili.cui@intel.com>
> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; binutils@sourceware.org
> Subject: Re: [PATCH 2/8] Support APX GPR32 with extend evex prefix
> 
> On 19.10.2023 18:38, Cui, Lili wrote:
> >>> @@ -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;
> > +       }
> 
> I guess my earlier response was ambiguous, I'm sorry: I didn't mean the
> disassembler's internal names, but the field names as per the SDM (e.g.
> EVEX.W). Also (nit) please avoid typo-ing "require" even twice.
> 
Changed.

+      /* EVEX from legacy instructions require that EVEX.L’L, EVEX.z and the
+        lower 2 bits of EVEX.aaa must be 0.
+        EVEX from evex instrucions require that EVEX.L’L and the lower 2 bits of
+        EVEX.aaa must be 0.  */

Thanks,
Lili.