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

["Jan Beulich" <JBeulich at suse dot com>]

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

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

Jan