Re: [PATCH 4/8] Support APX NDD

[Jan Beulich <jbeulich@suse.com>]

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.

>>> @@ -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.

Jan