Re: [PATCH v3 4/9] Support APX GPR32 with extend evex prefix

[Jan Beulich <jbeulich@suse.com>]

On 12.12.2023 13:58, Cui, Lili wrote:
>>
>>>>> @@ -14233,6 +14276,12 @@ static bool check_register (const reg_entry
>>>> *r)
>>>>>        if (!cpu_arch_flags.bitfield.cpuapx_f
>>>>>  	  || flag_code != CODE_64BIT)
>>>>>  	return false;
>>>>> +
>>>>> +      /* When using RegRex2, dual VEX/EVEX templates need to be
>>>>> + marked as
>>>> EVEX.
>>>>> +	 For the later install_template function.  */
>>>>> +      if (current_templates->start->opcode_modifier.vex
>>>>> +	  && current_templates->start->opcode_modifier.evex)
>>>>> +	i.vec_encoding = vex_encoding_evex;
>>>>
>>>> I'm afraid I don't understand the 2nd sentence of the comment. This
>>>> may be related to my question regarding cpu_flags_match() further up.
>>>>
>>>> The first sentence isn't quite correct either - you don't mark any
>>>> template here (and you can't, because we don't even know yet which
>>>> template we're going to use).
>>>>
>>>> Finally - do you really need the .evex check here? (I won't exclude
>>>> that this yields a better diagnostic in certain cases, but this wants
>>>> clarifying if so.)
>>>>
>>>
>>> If you look at install_template(), you'll see that before this function we
>> need to know if the current encoding is evex.
>>
>> "This function" being check_register()? If so, then no, we can't know up front
>> whether EVEX encoding is going to be needed, as operand parsing happens
>> ahead of template selection. If instead you mean "that function" and hence
>> install_template(), then yes, we need to know whether to use EVEX there.
>> Yet how does that result in a need for the .evex check here? (Or maybe your
>> reply was really to the first of the three parts of my earlier one?)
>>
> 
> Agree with you, put them here is unreasonable. 
> 
> For example 
> 
> vtestps (%r27),%ymm6
> 
> we should report unsupported  Egpr. But without .evex check, it will report "Error: no EVEX encoding for `vtestps'"
> 
>> But anyway - as said earlier on, using current_templates here looks wrong in
>> the first place. check_register() deals with only a register, without regard to
>> the context it is used in (with the sole exception of allow_pseudo_reg).
>> May I remind you that earlier on I already indicated that I suspect you'll need
>> a new enumerator to put in i.vec_encoding for this new purpose?
>>
> 
> If we don't put it in check_register(), we need to add a for loop at the beginning of the install_template() to check RegRex2. Do you think it is okay? Or create a function for it.
> 
> for (unsigned int op = 0; op < i.operands; op++)
>     {
>       if (i.types[op].bitfield.class != Reg)
>         continue;
> 
>       if (i.op[op].regs->reg_flags & RegRex2)
>         i.vec_encoding = vex_encoding_evex;
>     }
> 
>   if ((i.index_reg && (i.index_reg->reg_flags & RegRex2))
>       || (i.base_reg && (i.base_reg->reg_flags & RegRex2)))
>     i.vec_encoding = vex_encoding_evex; 

As a last resort this may be an option. But until my suggestion wasn't at
least tried or demonstrated to be worse, I don't think the above would be
acceptable.

Jan