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

[Jan Beulich <jbeulich@suse.com>]

On 13.12.2023 09:35, 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.
>>
> 
>>>> 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?
> 
> Jan, I didn't get your point, I think the enumerator vex_encoding_evex works well, the question is how to filter if Egpr is used in the current instruction, we should make a choice before install_template, whether it's an evex template or a vex template.

Well, of course you can make the existing enumerator work, by - see above -
adding yet another loop over all operands. In a similar way it may have
been possible to avoid the introduction of vex_encoding_evex512. But it is
generally better to record the precise reason for requiring a certain
encoding / putting in place a certain restriction, i.e. going even beyond
the desire to avoid introducing new loops over all operands if at all
possible.

Here as soon as an eGPR is used, various encodings cannot be used anymore.
That's best expressed explicitly. (It might even be that such a new
enumerator would help with the REX2 encodings. Recall that I said earlier
already that both field and enumerator names aren't fully appropriate
anymore. Yet changing them isn't a priority, so we can defer that until
after all the APX work has landed.)

Jan