RE: [PATCH 1/3] x86: Use vexvvvv to encode the vvvv register

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

> On 24.04.2024 09:23, Cui, Lili wrote:
> > gas/ChangeLog:
> >
> >         * config/tc-i386.c (optimize_encoding): Replaced 1 with VexVVVV_SRC1.
> >         (build_modrm_byte): Used vexvvvv to encode the vvvv register.
> >         (s_insn): Replaced 1 with VexVVVV_SRC1.
> >
> > opcodes/ChangeLog:
> >
> >         * i386-opc.h (VexVVVV_DST): Adjusted the value.
> >         (VexVVVV_SRC1): New.
> >         * i386-opc.tbl: Replaced VexVVVV with VexVVVV_SRC1.
> 
> Not quite: You replace by Src1VVVV now.
> 

OK.

> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -5045,7 +5045,7 @@ optimize_encoding (void)
> >         */
> >        i.tm.opcode_space = SPACE_0F;
> >        i.tm.base_opcode = 0x6c;
> > -      i.tm.opcode_modifier.vexvvvv = 1;
> > +      i.tm.opcode_modifier.vexvvvv = VexVVVV_SRC1;
> >
> >        ++i.operands;
> >        ++i.reg_operands;
> > @@ -10432,19 +10432,19 @@ build_modrm_byte (void)
> >  				     || i.encoding == encoding_evex));
> >      }
> >
> > -  if (i.tm.opcode_modifier.vexvvvv == VexVVVV_DST)
> > +  switch (i.tm.opcode_modifier.vexvvvv)
> >      {
> > -      v = dest;
> > -      dest-- ;
> > -    }
> > -  else
> > -    {
> > -      for (v = source + 1; v < dest; ++v)
> > -	if (v != reg_slot)
> > -	  break;
> > -      if (v >= dest)
> > -	v = ~0;
> 
> Replacing this by ...
> 
> > -    }
> > +    case VexVVVV_SRC1:
> > +      v =  dest - 1;
> > +      break;
> 
> ... just this could do with a word of explanation in the (sadly once again empty)
> description. While I'm sure this tests out okay for you, it's not immediately clear
> why the loop can be replaced this easily. Whereas by the end of the series (with
> SwapSources dropped) this simplification is pretty obvious.
> 

SDM said "VEX.vvvv encodes the first source register operand". It should be "dest -1". The old logic did not check vexvvvv first, which made the logic here very complicated. I'll add some comments here.

> As to the deecription once again being empty: This is even more of an issue
> considering that the title suggests the patch isn't doing anything.
> After all we already use vexvvvv for the stated purpose.
> 

The title is "Use vexvvvv to encode the vvvv register", which means that use vexvvvv for the stated purpose.

> Finally with the reg_slot use gone here (assuming, as per above, that this is
> actually correct at this point), that variable wants to move into the more
> narrow scope it's now solely used in, and it wants to have its initializer dropped.

Ok.

> > @@ -10462,10 +10462,7 @@ build_modrm_byte (void)
> >      }
> >
> >    if (v < MAX_OPERANDS)
> > -    {
> > -      gas_assert (i.tm.opcode_modifier.vexvvvv);
> > -      i.vex.register_specifier = i.op[v].regs;
> > -    }
> > +    i.vex.register_specifier = i.op[v].regs;
> 
> I have to admit I'm not entirely convinced of it being a good idea to drop the
> assertion. At least as long as the SWAP_SOURCES logic is still there.
> 

Ok, I will move it to the patch 2/3.

> >  vpgatherqq, 0x6691, AVX2, Modrm|Vex256|Space0F38|Src1VVVV|VexW1|SwapSources|NoSuf|VecSIB2
> 56, { RegYMM, Qword|Unspecified|BaseIndex, RegYMM }
> 
> Anything using SwapSources wants leaving alone in this patch. There's no point
> touching any of these twice (here and when subsequently you replace
> SwapSources).
> That'll also help to limit patch size a little.
> 

If we keep the old value VexVVVV here, we still need to deal with it in tc-i386.c and i386-opc.h, which is a bit strange.

Thanks,
Lili.