Re: [PATCH] x86: Determine vector length from the last vector operand

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Tue, Jul 24, 2018 at 4:55 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 24.07.18 at 13:38, <hjl.tools@gmail.com> wrote:
>> On Tue, Jul 24, 2018 at 12:41 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 22.07.18 at 21:02, <hjl.tools@gmail.com> wrote:
>>>> Determine VEX/EVEXE vector length from the last multi-length vector
>>>> operand.
>>>
>>> I see you've committed this already: It would have been really
>>> helpful to say _why_ you're doing the change in the commit message.
>>> For posterity as well as my understanding - could you at least do so
>>> here please? That's even more so that VEX and EVEX processing
>>> differed in that regard before your change.
>>
>> The encoding can be determined by the last  multi-length vector
>> operand.  There is no need to scan from the start.
>
> But why is scanning from the rear better? Immediates usually sit at the
> end. In the EVEX case, considering the break-s you add, this may
> indeed be more efficient, but the same then wouldn't hold for the VEX

break can be used only when scanning from the last operand.

> case.
>
>>>> --- a/gas/config/tc-i386.c
>>>> +++ b/gas/config/tc-i386.c
>>>> @@ -3363,10 +3363,12 @@ build_vex_prefix (const insn_template *t)
>>>>      vector_length = 1;
>>>>    else
>>>>      {
>>>> -      unsigned int op;
>>>> +      int op;
>>>
>>> This is the sort of change I would have objected to, btw. Variables
>>> used to index arrays should be unsigned whenever possible. And
>>> doing so would have been easy enough here:
>>
>> I want
>>
>> if (op < 0)
>>   abort ();
>>
>> after the loop.
>
>   if (op >= MAX_OPERANDS)
>     abort ()
>
> would be equivalent afaict.

It won't work when scanning from the last operand.


>>>> @@ -3611,20 +3613,31 @@ build_evex_prefix (void)
>>>>        if (!i.tm.opcode_modifier.evex
>>>>         || i.tm.opcode_modifier.evex == EVEXDYN)
>>>>       {
>>>> -       unsigned int op;
>>>> +       int op;
>>>>
>>>> +       /* Determine vector length from the last multi-length vector
>>>> +          operand.  */
>>>>         vec_length = 0;
>>>> -       for (op = 0; op < i.tm.operands; ++op)
>>>> +       for (op = i.operands - 1; op >= 0; op--)
>>>
>>>           for (op = i.operands; op--; )
>>>
>>> (similarly further down).
>>
>> They are equivalent.
>
> Exactly. I was just trying to make clear that the suggested change
> has no downside.
>
>>>>           if (i.tm.operand_types[op].bitfield.xmmword
>>>>               + i.tm.operand_types[op].bitfield.ymmword
>>>>               + i.tm.operand_types[op].bitfield.zmmword > 1)
>>>>             {
>>>>               if (i.types[op].bitfield.zmmword)
>>>> -               i.tm.opcode_modifier.evex = EVEX512;
>>>> +               {
>>>> +                 i.tm.opcode_modifier.evex = EVEX512;
>>>> +                 break;
>>>> +               }
>>>>               else if (i.types[op].bitfield.ymmword)
>>>> -               i.tm.opcode_modifier.evex = EVEX256;
>>>> +               {
>>>> +                 i.tm.opcode_modifier.evex = EVEX256;
>>>> +                 break;
>>>> +               }
>>>>               else if (i.types[op].bitfield.xmmword)
>>>> -               i.tm.opcode_modifier.evex = EVEX128;
>>>> +               {
>>>> +                 i.tm.opcode_modifier.evex = EVEX128;
>>>> +                 break;
>>>> +               }
>>>>               else if (i.broadcast && (int) op == i.broadcast->operand)
>>>
>>> This has rendered all the "else" pointless.
>>
>> What do you mean? Isn't it used in
>>
>> vcvttpd2uqq, 2, 0x6678, None, 1, CpuAVX512DQ,
>> Modrm|Masking=3|VexOpcode=0|VexW=2|Broadcast|Disp8ShiftVL|CheckRegSize|No_bS
>> uf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf,
>> { RegXMM|RegYMM|RegZMM|Qword|Unspecified|BaseIndex,
>> RegXMM|RegYMM|RegZMM }
>
> I didn't mean to drop the "else if (...)", just the "else" (with the
> if () to stay).
>

Scanning from the last operand is more efficient for both EVEX and VEX.
Please submit a patch if you can improve it.

-- 
H.J.