Re: [PATCH v5 4/5] x86: correct VFPCLASSP{S,D} operand size handling

[Jan Beulich <jbeulich at suse dot com>]

On 11.02.2020 12:49, H.J. Lu wrote:
> On Tue, Feb 11, 2020 at 2:26 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> With AVX512VL disabled (e.g. when writing code for the Knights family
>> of processors) these insns aren't ambiguous when used with a memory
>> source, and hence should be accepted without suffix or operand size
>> specifier. When AVX512VL is enabled, to be consistent with this as
>> well as other ambiguous operand size handling it would seem better to
>> just warn about the ambiguity in AT&T mode, and still default to 512-bit
>> operands (on the assumption that the code may have been written without
>> AVX512VL in mind yet), but it was requested to leave AT&T syntax mode
>> alone here.
>>
>> gas/
>> 2020-02-XX  Jan Beulich  <jbeulich@suse.com>
>>
>>         * config/tc-i386.c (avx512): New (at file scope), moved from
>>         (check_VecOperands): ... here.
>>         (process_suffix): Add [XYZ]MMword operand size handling.
>>         * testsuite/gas/i386/avx512dq-inval.s: Add VFPCLASS tests.
>>         * testsuite/gas/i386/noavx512-2.s: Add Intel syntax VFPCLASS
>>         tests.
>>         * testsuite/gas/i386/avx512dq-inval.l,
>>         testsuite/gas/i386/noavx512-2.l: Adjust expectations.
>>
>> opcodes/
>> 2020-02-XX  Jan Beulich  <jbeulich@suse.com>
>>
>>         * i386-opc.tbl (vfpclasspd, vfpclassps): Add Intel sytax form
>>         with Unspecified, making the present one AT&T syntax only.
>>         * i386-tbl.h: Re-generate.
>> ---
>> v5: Re-base.
>> v4: Restrict to just Intel syntax mode. Re-base.
>> v3: Re-base.
>>
>> --- a/gas/config/tc-i386.c
>> +++ b/gas/config/tc-i386.c
>> @@ -1840,6 +1840,8 @@ cpu_flags_and_not (i386_cpu_flags x, i38
>>    return x;
>>  }
>>
>> +static const i386_cpu_flags avx512 = CPU_ANY_AVX512F_FLAGS;
>> +
>>  #define CPU_FLAGS_ARCH_MATCH           0x1
>>  #define CPU_FLAGS_64BIT_MATCH          0x2
>>
>> @@ -5352,7 +5354,6 @@ check_VecOperands (const insn_template *
>>  {
>>    unsigned int op;
>>    i386_cpu_flags cpu;
>> -  static const i386_cpu_flags avx512 = CPU_ANY_AVX512F_FLAGS;
>>
>>    /* Templates allowing for ZMMword as well as YMMword and/or XMMword for
>>       any one operand are implicity requiring AVX512VL support if the actual
>> @@ -6447,7 +6448,8 @@ process_suffix (void)
>>        /* Accept FLDENV et al without suffix.  */
>>        && (i.tm.opcode_modifier.no_ssuf || i.tm.opcode_modifier.floatmf))
>>      {
>> -      unsigned int suffixes;
>> +      unsigned int suffixes, evex = 0;
>> +      i386_cpu_flags cpu;
>>
>>        suffixes = !i.tm.opcode_modifier.no_bsuf;
>>        if (!i.tm.opcode_modifier.no_wsuf)
>> @@ -6461,7 +6463,55 @@ process_suffix (void)
>>        if (flag_code == CODE_64BIT && !i.tm.opcode_modifier.no_qsuf)
>>         suffixes |= 1 << 5;
>>
>> -      /* Are multiple suffixes allowed?  */
>> +      /* For [XYZ]MMWORD operands inspect operand sizes.  */
>> +      cpu = cpu_flags_and (i.tm.cpu_flags, avx512);
>> +      if (!cpu_flags_all_zero (&cpu) && !i.broadcast)
>> +       {
>> +         unsigned int op;
>> +
>> +         for (op = 0; op < i.tm.operands; ++op)
>> +           {
>> +             if (!cpu_arch_flags.bitfield.cpuavx512vl)
>> +               {
>> +                 if (i.tm.operand_types[op].bitfield.ymmword)
>> +                   i.tm.operand_types[op].bitfield.xmmword = 0;
>> +                 if (i.tm.operand_types[op].bitfield.zmmword)
>> +                   i.tm.operand_types[op].bitfield.ymmword = 0;
>> +                 if (!i.tm.opcode_modifier.evex
>> +                     || i.tm.opcode_modifier.evex == EVEXDYN)
>> +                   i.tm.opcode_modifier.evex = EVEX512;
>> +               }
>> +
>> +             if (i.tm.operand_types[op].bitfield.xmmword
>> +                 + i.tm.operand_types[op].bitfield.ymmword
>> +                 + i.tm.operand_types[op].bitfield.zmmword < 2)
>> +               continue;
>> +
>> +             /* Any properly sized operand disambiguates the insn.  */
>> +             if (i.types[op].bitfield.xmmword
>> +                 || i.types[op].bitfield.ymmword
>> +                 || i.types[op].bitfield.zmmword)
>> +               {
>> +                 suffixes &= ~(7 << 6);
>> +                 evex = 0;
>> +                 break;
>> +               }
>> +
>> +             if ((i.flags[op] & Operand_Mem)
>> +                 && i.tm.operand_types[op].bitfield.unspecified)
>> +               {
>> +                 if (i.tm.operand_types[op].bitfield.xmmword)
>> +                   suffixes |= 1 << 6;
>> +                 if (i.tm.operand_types[op].bitfield.ymmword)
>> +                   suffixes |= 1 << 7;
>> +                 if (i.tm.operand_types[op].bitfield.zmmword)
>> +                   suffixes |= 1 << 8;
>> +                 evex = EVEX512;
>> +               }
>> +           }
>> +       }
>> +
>> +      /* Are multiple suffixes / operand sizes allowed?  */
>>        if (suffixes & (suffixes - 1))
>>         {
>>           if (intel_syntax
>> @@ -6491,6 +6541,8 @@ process_suffix (void)
>>                    || (i.tm.base_opcode == 0x63
>>                        && i.tm.cpu_flags.bitfield.cpu64))
>>             /* handled below */;
>> +         else if (evex)
>> +           i.tm.opcode_modifier.evex = evex;
>>           else if (flag_code == CODE_16BIT)
>>             i.suffix = WORD_MNEM_SUFFIX;
>>           else if (!i.tm.opcode_modifier.no_lsuf)
> 
> So this change only impacts Intel syntax with AVX512VL disabled.  Why
> are there so many
> assembler changes?

There aren't "many" changes, it's just one larger block of code that
needs adding (paralleling the suffix checking for GPR(-like) operands).

>  If they are really needed, can you make them
> conditioned on Intel
> syntax?

Well, that block of code is the meat of the change, so yes, it is
needed. Some of what it does may also be beneficial for AT&T mode,
but yes, I think I can make all of it Intel-syntax only (with a
comment saying why this is).

Jan