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

[Jan Beulich <jbeulich@suse.com>]

On 13.11.2023 06:53, Cui, Lili wrote:
>> Subject: Re: [PATCH V2 3/8] Support APX GPR32 with extend evex prefix
>>
>> On 03.11.2023 17:50, Cui, Lili wrote:
>>> @@ -3885,6 +3885,14 @@ is_any_vex_encoding (const insn_template *t)
>>>    return t->opcode_modifier.vex || is_evex_encoding (t);  }
>>>
>>> +static INLINE bool
>>> +is_any_apx_evex_encoding (void)
>>> +{
>>> +  return i.rex2 || i.tm.opcode_space == SPACE_EVEXMAP4
>>> +    || (i.vex.register_specifier
>>> +	&& i.vex.register_specifier->reg_flags & RegRex2); }
>>
>> The use of i.rex2 here doesn't fit the name; the sole user has first checked
>> that no legacy encoding is going to be used, and that's a prereq here. Such a
>> prereq needs spelling out, such that one can be easily aware when possibly
>> adding another caller.
>>
>> Also, what does "any" stand for in the name here ...
>>
> 
> How about "check_if_any_vex_is_evex_apx_encoding ()" ?

That still has "any" in the name for an unexplained reason, and the
new name is yet longer and hence yet more difficult to follow. My
question was rather towards simply dropping the "any" from the name.
Unless of course you can clarify what "any" means there.

>>> @@ -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?

>>> @@ -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.

>>> @@ -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.

Jan