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

[Jan Beulich <jbeulich@suse.com>]

On 14.11.2023 04:12, Cui, Lili wrote:
>> On 13.11.2023 06:53, Cui, Lili wrote:
>>>>> @@ -5624,19 +5653,42 @@ md_assemble (char *line) [...]
>>>>> -      if (i.tm.opcode_modifier.vex)
>>>>> +      if (is_any_apx_evex_encoding ())
>>>>> +	{
>>>>> +	  if (i.tm.opcode_space == SPACE_EVEXMAP4 &&
>>>> (i.prefix[DATA_PREFIX] != 0))
>>>>> +	    {
>>>>> +	      i.tm.opcode_modifier.opcodeprefix = PREFIX_0X66;
>>>>
>>>> Perhaps better assert that no other embedded prefix was already
>>>> recorded here?
>>>
>>> Added the code as below, I added as_bad instead of assert, I think this is a
>> input error and not a gas internal error, right?  Besides REX_PREFIX?
>>>
>>>       if (check_if_any_vex_is_evex_apx_encoding ())
>>>         {
>>>           if (i.tm.opcode_space == SPACE_EVEXMAP4 &&
>> (i.prefix[DATA_PREFIX] != 0))
>>>             {
>>>
>>>               i.tm.opcode_modifier.opcodeprefix = PREFIX_0X66;
>>>               i.prefix[DATA_PREFIX] = 0;
>>>
>>>               /*  Prefixes other than the rex prefix cannot be used with the data
>> prefix.  */
>>>               const unsigned char *p = i.prefix;
>>>
>>>               for (j = 0; j < ARRAY_SIZE (i.prefix); ++j, ++p)
>>>                 {
>>>                   if (!*p)
>>>                     continue;
>>>
>>>                   switch (j)
>>>                     {
>>>                     case DATA_PREFIX:
>>>                     case REX_PREFIX:
>>>                       break;
>>>                     default:
>>>                       as_bad (_("unexpecting prefix %x together with DATA "
>>>                                 "prefix in front of evex-promoted apx "
>>>                                 "instruction "), *p);
>>>                       return;
>>>                     }
>>>                 }
>>>             }
>>>
>>>           build_apx_evex_prefix ();
>>>       }
>>
>> We already have code refusing most legacy prefixes when ahead of VEX/EVEX,
>> don't we? I'd expect that code to take care of bogus DATA prefixes here as
>> well. Possibly the conversion needs dealing with differently if here we can't
>> tell a user-supplied i.prefix[DATA_PREFIX] from one internally derived from
>> operands which were supplied? E.g. by not having
>> process_suffix() invoke add_prefix() in this particular case at all, but instead
>> modify i.tm accordingly?
>>
> 
> Done.
> 
> -         if (!add_prefix (prefix))
> -           return 0;
> +         /* The DATA PREFIX of EVEX promoted from legacy APX instructions
> +            needs to be adjusted.  */
> +         if (i.tm.opcode_space == SPACE_EVEXMAP4)
> +           i.tm.opcode_modifier.opcodeprefix = PREFIX_0X66;
> +         else
> +           if (!add_prefix (prefix))
> +             return 0;

Except that you want "else" and "if" on the same line, please.

>>>>> @@ -7043,7 +7096,7 @@ VEX_check_encoding (const insn_template *t)
>>>>> static int  check_EgprOperands (const insn_template *t)  {
>>>>> -  if (t->opcode_modifier.noegpr)
>>>>> +  if (t->opcode_modifier.noegpr && !need_evex_encoding())
>>>>>      {
>>>>>        for (unsigned int op = 0; op < i.operands; op++)
>>>>>  	{
>>>>
>>>> What is this change about?
>>>>
>>>
>>> After merging vex and evex, evex exits here early and all evex supports egpr.
>>
>> Yet / hence no EVEX template should ever have NoEgpr set. IOW I still don't
>> follow why this change would be needed.
>>
> 
> After merging VEX and EVEX templates, they shared NoEgpr ==1 , and if !EVEX is not added, It will check for NoEgpr for EVEX instructions, but EVEX supports egpr.  That's why I added the code.
> 
> ldtilecfg, 0x49/0, AMX_TILE&(AMX_TILE|APX_F), Modrm|Vex128|EVex128|Space0F38|VexW0|NoSuf|NoEgpr, { Unspecified|BaseIndex }
> 
> When merging VEX and EVEX templates, it means that VEX +Egpr (illegal) has a corresponding EVEX + Egpr, and we don't need to check whether VEX is illegal (just set NoEgpr =0 ). Then we can remove "!need_evex_encoding()" and the following comments code. IOW merging templates means we cannot differentiate their NoEgpr.

Well, such merged templates necessarily allow Egpr, so they must not have NoEgpr.
For them, use of an extended register simply excludes use of the VEX form of the
insn. This may or may not be what the last paragraph of your reply is intended to
be telling me.

>>>>> @@ -14252,6 +14306,9 @@ static bool check_register (const reg_entry
>>>>> *r)
>>>>>
>>>>>    if (r->reg_flags & RegRex2)
>>>>>      {
>>>>> +      if (is_evex_encoding (current_templates->start))
>>>>> +	i.vec_encoding = vex_encoding_evex;
>>>>
>>>> What if the APX template isn't first in the group?
>>>>
>>>
>>> If apx_f is not supported, it will return false, just after this code. Oh, better
>> to move it to the back. Done.
>>>
>>>   if (r->reg_flags & RegRex2)
>>>     {
>>>       if (current_templates->start->opcode_modifier.evex)
>>>         i.vec_encoding = vex_encoding_evex;
>>>
>>>       if (!cpu_arch_flags.bitfield.cpuapx_f
>>>           || flag_code != CODE_64BIT)
>>>         return false;
>>>     }
>>
>> Hmm, as before there's a use of current_templates here, which I'm afraid isn't
>> appropriate. Whether a register is legitimate to use depends on only the
>> present mode we're assembling in (flag_code + cpu_arch_flags, plus a few
>> other globals for certain special cases). There may not be dependencies on
>> the insn we're processing. Or if at all (yet even that would need a pretty good
>> justification), then only on the collective set of all templates in the chosen
>> template group.
> 
> The code here is need by the upper comments.

I'm afraid I don't understand your reply here. It certainly doesn't address my
comment. Why would setting i.vec_encoding depend on the first template in a
group? I indicated earlier that using vex_encoding_evex may not be feasible
here, because you don't mean to enforce EVEX encoding. You only want to record
the fact that an encoding needs using which is eGPR-capable (i.e. either one
with NoEgpr clear, or an EVEX one). Hence why I further suggested to possibly
introduce vex_encoding_egpr. (The "vex" in the names and the "vec" in the
field name are increasingly misleading, but we can address that later on.)

Jan