[PATCH 4/8] Support APX NDD

[Cui, Lili]

> Subject: Re: [PATCH 4/8] Support APX NDD
> 
> On 24.10.2023 13:39, Cui, Lili wrote:
> >> Subject: Re: [PATCH 4/8] Support APX NDD
> >>
> >> On 19.09.2023 17:25, Cui, Lili wrote:
> >>> --- a/opcodes/i386-dis-evex-prefix.h
> >>> +++ b/opcodes/i386-dis-evex-prefix.h
> >>> @@ -353,8 +353,8 @@
> >>>    /* PREFIX_EVEX_MAP4_66 */
> >>>    {
> >>>      { MOD_TABLE (MOD_EVEX_MAP4_66_PREFIX_0) },
> >>> -    { "adoxS",	{ Gdq, Edq }, 0 },
> >>> -    { "adcxS",	{ Gdq, Edq }, 0 },
> >>> +    { "adoxS",	{ VexGdq, Gdq, Edq }, 0 },
> >>> +    { "adcxS",	{ VexGdq, Gdq, Edq }, 0 },
> >>
> >> With the OP_VEX() change moved to the earlier patch, you wouldn't
> >> need to alter again right away what was just added there.
> >>
> >
> > As I commented in another email, I want to move instructions like adcx into
> this NDD patch. Do you think it is ok?
> 
> Yeah, having ND-capable insns in the ND patch makes sense. I don't care overly
> much about the actual split. It wants to be half-way consistent, and beyond
> that - as said - it helps if code/data added by earlier patches doesn't need re-
> writing altogether in later ones.
> 

Ok.

> >>> @@ -13280,6 +13304,14 @@ OP_VEX (instr_info *ins, int bytemode, int
> >> sizeflag ATTRIBUTE_UNUSED)
> >>>    if (!ins->need_vex)
> >>>      return true;
> >>>
> >>> +  if (ins->evex_type == evex_from_legacy)
> >>> +    {
> >>> +      if (ins->vex.b)
> >>> +	ins->evex_used |= EVEX_b_used;
> >>> +      else
> >>> +	 return true;
> >>> +    }
> >>
> >> When you reuse fields or definitions in places where their names
> >> don't match their purpose (the field dealt with here is "nd" after
> >> all, not "b"), a comment wants adding. There's also something odd
> >> with indentation here, but I suppose an if/else construct isn't needed in the
> first place.
> >>
> > Added comment and adjusted the formatting, I think we need if/else like
> "adox " which supports EVEX.ND=0/1.
> 
> You will need if(), sure, but I'm not convinced of "else". Constructs like
> 
>     if (cond)
>       return;
>     else
>       whatever;
> 
> bother me in general, as adding (even if just slightly) more mental load for the
> reader. Just
> 
>     if (cond)
>       return;
>     whatever;
> 
> is all you need: Less text, less indentation. In the specific case above yet
> another possibility may exist (without having actually tried
> it):
> 
>   if (ins->evex_type == evex_from_legacy)
>     {
>       ins->evex_used |= EVEX_b_used;
>       if (!ins->vex.b)
> 	 return true;
>     }
> 
> I.e. this would be as long as setting EVEX_b_used without EVEX.b set doesn't
> cause any issues.
> 

Done.

Thanks,
Lili.