[PATCH v4 0/9] Support Intel APX EGPR

[Cui, Lili]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, December 20, 2023 4:57 PM
> To: Cui, Lili <lili.cui@intel.com>
> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; binutils@sourceware.org
> Subject: Re: [PATCH v4 0/9] Support Intel APX EGPR
> 
> On 20.12.2023 09:50, Cui, Lili wrote:
> >> On 19.12.2023 13:12, Cui, Lili wrote:
> >>> *** BLURB HERE ***
> >>> Future optimizations to be made.
> >>> 1. The current implementation of vexvvvvv needs to be optimized.
> >>> 2. The handling of double VEX/EVEX templates in check_register()
> >>> needs to
> >> be optimized.
> >>
> >> I hope this is just stale here, and the dependency on templates was
> >> now removed again from check_register().
> >
> > In fact, I didn't remove it in V4, I didn't find a better place to deal with it. I
> don't know if you agree with this implementation below.
> 
> I'm afraid I don't, both because it still isn't clear to me what's wrong with my
> alternative proposal, and also for the formal reason of ...
> 

For the alternative proposal, do you mean adding a new variable to avoid introducing new loops over all operands? How about this ? or do you want to add other variable and handle it in check_register?

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -464,6 +464,9 @@ struct _i386_insn
     /* Have NOTRACK prefix.  */
     const char *notrack_prefix;

+    /* Has Egpr.  */
+    bool has_egpr;
+
     /* Error message.  */
     enum i386_error error;
   };
@@ -3683,7 +3686,7 @@ install_template (const insn_template *t)
           || maybe_cpu (t, CpuFMA))
          && (maybe_cpu (t, CpuAVX512F) || maybe_cpu (t, CpuAVX512VL)))
        {
-         if (need_evex_encoding ())
+         if (need_evex_encoding () || i.has_egpr)
            {
              i.tm.opcode_modifier.vex = 0;
              i.tm.cpu.bitfield.cpuavx512f = i.tm.cpu_any.bitfield.cpuavx512f;
@@ -3704,7 +3707,7 @@ install_template (const insn_template *t)
       if (APX_F(CpuCMPCCXADD) || APX_F(CpuAMX_TILE) || APX_F(CpuAVX512F)
          || APX_F(CpuAVX512DQ) || APX_F(CpuAVX512BW) || APX_F(CpuBMI)
          || APX_F(CpuBMI2))
-       if (need_evex_encoding ())
+       if (need_evex_encoding () || i.has_egpr)
          i.tm.opcode_modifier.vex = 0;
        else
          i.tm.opcode_modifier.evex = 0;
@@ -14523,11 +14526,12 @@ static bool check_register (const reg_entry *r)
          || flag_code != CODE_64BIT)
        return false;

+      i.has_egpr = true;


Lili.

> > /* For dual VEX/EVEX templates, evex encoding is required when the input
> has
> >    egpr.*/
> > static INLINE void
> > vex_with_Egpr_requires_evex_encoding (const insn_template *t) {
> >   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;
> 
> ... it not being okay to override i.vec_encoding like this, when it may already
> have been set to another value.
> 
> Jan
> 
> >     }
> >
> >   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; }
> >
> > static INLINE void
> > install_template (const insn_template *t) {
> >   unsigned int l;
> >
> >   i.tm = *t;
> >
> >   /* Dual VEX/EVEX templates need stripping one of the possible variants.  */
> >   if (t->opcode_modifier.vex && t->opcode_modifier.evex)
> >     {
> >       vex_with_Egpr_requires_evex_encoding (t);
> >
> >
> > Regards,
> > Lili.