Re: [PATCH] Support APX NF

[Jan Beulich <jbeulich@suse.com>]

On 29.02.2024 14:47, Cui, Lili wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: Thursday, February 29, 2024 8:04 PM
>>>> To: Cui, Lili <lili.cui@intel.com>
>>>> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; binutils@sourceware.org
>>>> Subject: Re: [PATCH] Support APX NF
>>>>
>>>> On 29.02.2024 13:00, Cui, Lili wrote:
>>>>>> On 27.02.2024 10:01, Cui, Lili wrote:
>>>>>>> @@ -8860,6 +8880,9 @@ match_template (char mnem_suffix)
>>>>>>>  		  goto check_operands_345;
>>>>>>>  		}
>>>>>>>  	      else if (t->opcode_space != SPACE_BASE
>>>>>>> +		       /* Map0 and map1 are promoted to MAP4 when NF is
>>>>>> enabled.
>>>>>>> +			*/
>>>>>>> +		       && !t->opcode_modifier.nf
>>>>>>>  		       && (t->opcode_space != SPACE_0F
>>>>>>>  			   /* MOV to/from CR/DR/TR, as an exception, follow
>>>>>>>  			      the base opcode space encoding model.  */
>>>>>>
>>>>>> I don't understand this: How does a template permitting NF matter here?
>>>>>> I could see the immediately preceding "else if" become something
>>>>>> along the lines of
>>>>>>
>>>>>> 	      else if (is_cpu (t, CpuAPX_F) && (i.operands == 3 ||
>>>>>> i.has_nf))
>>>>>>
>>>>>> But I admit I didn't fully think this through. It's just that the
>>>>>> change as is looks wrong to me.
>>>>>>
>>>>>
>>>>> I was also dissatisfied with this place yesterday and then modified it to:
>>>>>
>>>>>               else if (t->opcode_space != SPACE_BASE
>>>>>                        /* For EVEX-promoted instructions, opcode_space is
>>>>>                           promoted to MAP4.  */
>>>>>                        && (t->opcode_space != SPACE_EVEXMAP4
>>>>>                            || t->mnem_off == MN_movbe)
>>>>>                        && (t->opcode_space != SPACE_0F
>>>>>                            /* MOV to/from CR/DR/TR, as an exception, follow
>>>>>                               the base opcode space encoding model.  */
>>>>>                            || (t->base_opcode | 7) != 0x27))
>>>>>
>>>>> For EVEX-promoted instructions, opcode_space is promoted to MAP4.
>>>>> The
>>>> old judgment no longer fit for EVEX promoted instructions. However,
>>>> the logic of this place is still not good.
>>>>
>>>> So what about my suggestion?
>>>
>>> Your suggestion is better, but we need to replace i.has_nf with new
>> judgment, i.has_nf cannot cover instructions like adc, the testcase will be
>> added by another patch.
>>>
>>> +             else if (is_cpu (t, CpuAPX_F)
>>> +                      && (i.operands == 3 || (t->opcode_space == SPACE_EVEXMAP4
>>> +                                              && t->mnem_off !=
>>> + MN_movbe)))
>>
>> Ah yes, to cover the non-NF 2-operand forms. But then is the "i.operands ==
>> 3"
>> part actually still needed?
> 
> It can be removed,  great suggestion, thanks!

Thinking of it - the is_cpu() then likely is unnecessary as well. If new
map4 insn appeared that have D set in the template, they would likely
follow the same pattern, yet be keyed to a different CPU feature. What's
less clear is whether instead of special-casing MOVBE it would make sense
to check the template's W bit here. Thoughts?

Jan