RE: [PATCH 5/8] Support APX NDD

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

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, November 23, 2023 6:58 PM
> To: Cui, Lili <lili.cui@intel.com>
> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; ccoutant@gmail.com;
> binutils@sourceware.org; Kong, Lingling <lingling.kong@intel.com>
> Subject: Re: [PATCH 5/8] Support APX NDD
> 
> On 22.11.2023 11:45, Cui, Lili wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Wednesday, November 22, 2023 4:48 PM
> >> To: Cui, Lili <lili.cui@intel.com>
> >> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; ccoutant@gmail.com;
> >> binutils@sourceware.org; Kong, Lingling <lingling.kong@intel.com>
> >> Subject: Re: [PATCH 5/8] Support APX NDD
> >>
> >> On 22.11.2023 08:46, Cui, Lili wrote:
> >>>>>>> @@ -9087,7 +9104,7 @@ get_valid_dis386 (const struct dis386
> *dp,
> >>>>>> instr_info *ins)
> >>>>>>>  	return &err_opcode;
> >>>>>>>
> >>>>>>>        /* Set vector length.  */
> >>>>>>> -      if (ins->modrm.mod == 3 && ins->vex.b)
> >>>>>>> +      if (ins->modrm.mod == 3 && ins->vex.b && ins->evex_type
> >>>>>>> + ==
> >>>>>>> + evex_default)
> >>>>>>>  	ins->vex.length = 512;
> >>>>>>>        else
> >>>>>>>  	{
> >>>>>>
> >>>>>> Is this change really needed for anything?
> >>>>>
> >>>>> If it's NDD and ins->vex.b ==1, we need to avoid giving NDD a
> >>>>> wrong
> >> value.
> >>>>
> >>>> But this is recording ->vex.length, not anything NDD related (afaics).
> >>>
> >>> There are some instructions that use OP_VEX, which will use ->vex.length.
> >>>
> >>> For example:
> >>> "addB",             { VexGb, Eb, Gb }
> >>
> >> But that's a GPR, for which ->vex.length is not supposed to have an effect.
> >>
> >
> > For EVEX-promoted instructions,  evex.ll == 0b00, which has the same
> encoding as vex.length == 128 and they can share the same processing with -
> >vex.length, ->vex.length also handles GPR in OP_VEX.
> >
> >
> >   switch (ins->vex.length)
> >     {
> >     case 128:
> >       switch (bytemode)
> >         {
> >         case x_mode:
> >           names = att_names_xmm;
> >           ins->evex_used |= EVEX_len_used;
> >           break;
> >         case v_mode:
> >         case dq_mode:
> >           if (ins->rex & REX_W)
> >             names = att_names64;
> >           else if (bytemode == v_mode
> >                    && !(sizeflag & DFLAG))
> >             names = att_names16;
> >           else
> >             names = att_names32;
> >           break;
> >         case b_mode:
> >           names = att_names8rex;
> >           break;
> >         case q_mode:
> >           names = att_names64;
> >           break;
> >         case mask_bd_mode:
> >         case mask_mode:
> >           if (reg > 0x7)
> >             {
> >               oappend (ins, "(bad)");
> >               return true;
> >             }
> >           names = att_names_mask;
> >           break;
> >         default:
> >           abort ();
> >           return true;
> >         }
> >       break;
> 
> Hmm, okay, I see that this is then down to an anomaly in pre-existing code.
> I don't think any of what's quoted above should actually depend on
> ->vex.length; it'll surely need to change once the first
> ->VEX/EVEX-encoded
> insns appears while has .l / .ll != 0 but still encodes a GPR. Yet then nothing I
> can sensibly demand you fix up front. Without which what I'd like to ask for
> (re-iterating earlier remarks): For anything not obvious (which this falls under),
> please add some explanation to the patch description.
> The general issue there is that while ChangeLog entries enumerate what is
> being done, they hardly ever say _why_ certain changes are needed.
> 
Thank you for your suggestion,  comments are really important. These patches were not written by me, when you asked about many strange places without comments, I actually couldn’t understand them either. I could only remove the code and compile it to extract the information from the wrong testcase.  I also realized the importance of comments.

Regards,
Lili.