Re: [PATCH 5/8] Support APX NDD

[Jan Beulich <jbeulich@suse.com>]

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.

Jan