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

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

> >>> --- 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.
> 
> Well, yes and no: We did so before already, so the title suggests "no change".
> 

Do you want to split this patch 1/3 into two? Or move it to 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.
> 
> VexVVVV without a value is the same as Src1VVVV, hence I don't think tc-
> i386.c would be affected by leaving those in place.
> 

OK.

Thanks,
Lili.