Re: [PATCH 2/8] Support APX GPR32 with extend evex prefix

[Jan Beulich <jbeulich@suse.com>]

On 18.10.2023 12:44, Cui, Lili wrote:
>> Without seeing how this is used I can't comment on it. It feels though as if you
>> may not have fully understood my earlier reply: Even prior to APX we already
>> have cases where one CPU specifier in the opcode table using | means "both"
>> and another means "either". I think we want to split that, and thus simplify
>> the logic in cpu_flags_match(). That's separate prereq work, of course.
>>
>> Thing is that prior to 734dfd1cc966 this would have been prohibitively
>> expensive in terms of table size growth. But now we can afford having two
>> i386_cpu_attr fields, one meaning "all of these", the other meaning "any of
>> these". To limit churn in the opcode table, I'd be inclined to continue to
>> express CPU requirements in a single field there, using e.g.
>> (CpuA|CpuB)&CpuC&CpuD (Cpu prefixes re-added here for clarity, even if
>> they aren't present in the opcode table anymore).
>>
>> I'd be happy to do that prereq work (if we can agree on the approach), but it
>> may mean a little bit of delay, as after my vacation I need to catch up with a
>> few other thinghs first.
>>
> 
> It would be great if you could do this (agree with the approach), it is not hurry, it may take a long time to commit the APX  to the master.
> 
>>>>> +  if (i.prefix[DATA_PREFIX] != 0)
>>>
>>>>> +    {
>>>
>>>>> +      i.tm.opcode_modifier.opcodeprefix = PREFIX_0X66;
>>>
>>>>> +      i.prefix[DATA_PREFIX] = 0;
>>>
>>>>> +    }
>>>
>>>>
>>>
>>>> While this looks to be correct for the case when the prefix was
>>>> derived from an
>>>
>>>> insn template and the use of 16-bit operands, I don't think it is
>>>> uniformly
>>>
>>>> correct when "data16" was used as a prefix explicitly. In such a case
>>>> either
>>>
>>>> REX2 encoding needs to be used, or an error needs emitting.
>>>
>>>>
>>>
>>>> You may further want to assert that i.tm.opcode_modifier.opcodeprefix
>>>> is still
>>>
>>>> zero ahead of the assignment.
>>>
>>>>
>>>
>>>
>>>
>>> For REX2 encoding, we add no special handling, just follow REX.
>>>
>>> For EVEX-promoted encoding, such as “data16
>> aand   %r25d,0x123(%r31,%rax,4)”, the following existing code will report an
>> error.
>>>
>>>
>>>
>>>   if (is_any_vex_encoding (&i.tm)
>>>
>>>       || i.tm.operand_types[i.imm_operands].bitfield.class >= RegMMX
>>>
>>>       || i.tm.operand_types[i.imm_operands + 1].bitfield.class >=
>>> RegMMX)
>>>
>>>     {
>>>
>>>       /* Check for data size prefix on VEX/XOP/EVEX encoded and SIMD
>>> insns.  */
>>>
>>>       if (i.prefix[DATA_PREFIX])
>>>
>>>         {
>>>
>>>           as_bad (_("data size prefix invalid with `%s'"), insn_name
>>> (&i.tm));
>>>
>>>           return;
>>>
>>>         }
>>
>> Thinking of it, I may need to revise my earlier comment some: RAO-INT insns
>> are a bad example here, since despite being legacy-encoded they don't permit
>> a data16 prefix to specify 16-bit operand size. Consider the same for e.g.
>> AND. The legacy form permits use of data16 (leaving aside that it's better /
>> clearer to simply use 16-bit register names), so the promoted forms likely
>> ought to permit such as well. IOW perhaps the code you have is correct, but
>> the check you quote may need adjusting.
>>
> 
> I listed 5 instructions to check the data16 prefix, is the last one what you want?
> 
> $ cat add.s
> 
> and %ebx, %eax
> data16 and %ebx, %eax
> and %ebx, %r16d
> data16 and %ebx, %r16d
> {evex} data16 and %ebx, %r16d
> 
> $ objdump -dw  and.o
> 
>    0:   21 d8                   and    %ebx,%eax
>    2:   66 21 d8                and    %bx,%ax
>    5:   d5 10 21 d8             and    %ebx,%r16d
>    9:   66 d5 10 21 d8          and    %bx,%r16w
>    Error: data size prefix invalid with `and'

Kind of. I was really thinking of e.g.

data16 and %ebx, %ecx, %edx

but yes, forms with {evex} would be similarly affected.

Jan