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

["Cui, Lili" <lili.cui@intel.com>]

> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; binutils@sourceware.org
> Subject: Re: [PATCH V2 3/8] Support APX GPR32 with extend evex prefix
> 
> 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.
> 

Done.

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

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

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

Thanks, 
Lili.