RE: [PATCH 1/8] Support APX GPR32 with rex2 prefix

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

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, November 6, 2023 11:03 PM
> To: Cui, Lili <lili.cui@intel.com>
> Cc: Lu, Hongjiu <hongjiu.lu@intel.com>; ccoutant@gmail.com;
> binutils@sourceware.org
> Subject: Re: [PATCH 1/8] Support APX GPR32 with rex2 prefix
> 
> On 02.11.2023 12:29, Cui, Lili wrote:
> > @@ -1119,6 +1148,18 @@ process_i386_opcode_modifier (FILE *table, char
> *mod, unsigned int space,
> >  	fprintf (stderr,
> >  		 "%s: %d: W modifier without Word/Dword/Qword
> operand(s)\n",
> >  		 filename, lineno);
> > +
> > +      /* The part about judging EVEX encoding should be synchronized with
> > +	 is_evex_encoding.  */
> > +      if (modifiers[Vex].value
> > +	  || ((space > SPACE_0F || has_special_handle)
> > +	      && !modifiers[EVex].value
> > +	      && !modifiers[Disp8MemShift].value
> > +	      && !modifiers[Broadcast].value
> > +	      && !modifiers[Masking].value
> > +	      && !modifiers[SAE].value))
> > +	modifiers[NoEgpr].value = 1;
> 
> While this is just i386-gen (and hence being somewhat inefficient isn't the end
> of the world) I still wonder whether we need all the parts of this condition:
> Do we really need all the constituents of this EVEX related checks? Wouldn't it
> also help is_evex_encoding() if we switched to uniformly having EVex
> attributes on all EVEX templates? A presently missing EVex attribute, after all,
> merely is another way of saying EVexDYN, if I'm not mistaken. (Such an
> adjustment, if deemed to help, would of course want to come as a separate,
> prereq patch.)
> 

Yes, EVex is another way of saying EVexDYN, it should be appear in every EVEX template, when we merge EVex128, EVex256 and EVex512 into one template we omitted the expression of EVexDYN. So some EVEX templates don’t have this tag. If we want to re-add it, we need new values.

Such as:
vcvttps2dq, 0xF35B, AVX512F, Modrm|Masking|Space0F|VexW0|Broadcast|Disp8ShiftVL|CheckOperandSize|NoSuf|SAE, { RegXMM|RegYMM|RegZMM|Dword|Unspecified|BaseIndex, RegXMM|RegYMM|RegZMM }

> Furthermore, is this correct at all for mixed VEX/EVEX templates?
> 
After merging the templates we only have one entry and I prefer to set [NoEgpr].value to 1. Don't check NoEgpr for all EVEX instruction in check_EgprOperands function. 

check_EgprOperands (const insn_template *t)
 {
-  if (t->opcode_modifier.noegpr)
+  if (t->opcode_modifier.noegpr && !need_evex_encoding())


> > --- a/opcodes/i386-opc.tbl
> > +++ b/opcodes/i386-opc.tbl
> > @@ -891,7 +891,7 @@ rex.wrxb, 0x4f, x64, NoSuf|IsPrefix, {}
> > <pseudopfx:ident:cpu, disp8:Disp8:0, disp16:Disp16:0, disp32:Disp32:0, +
> >                        load:Load:0, store:Store:0, +
> >                        vex:VEX:0, vex2:VEX:0, vex3:VEX3:0, evex:EVEX:0, +
> > -                      rex:REX:x64, nooptimize:NoOptimize:0>
> > +                      rex:REX:x64, rex2:REX2:x64,
> > + nooptimize:NoOptimize:0>
> 
> Seeing this I realized that there's something missing here (an APX_F
> dependency), which then again would not have had an effect without the
> patch [1] sent earlier today.
> 
> Jan
> 
> [1] https://sourceware.org/pipermail/binutils/2023-November/130345.html

Changed to 

+#define APX_F_64 APX_F|x64
+
<pseudopfx:ident:cpu, disp8:Disp8:0, disp16:Disp16:0, disp32:Disp32:0, +
                       load:Load:0, store:Store:0, +
                       vex:VEX:0, vex2:VEX:0, vex3:VEX3:0, evex:EVEX:0, +
-                      rex:REX:x64, rex2:REX2:x64, nooptimize:NoOptimize:0>
+                      rex:REX:x64, rex2:REX2:APX_F_64, nooptimize:NoOptimize:0>

When we have" x86: split insn templates' CPU field" in trunk, I will change it to #define APX_F_64 APX_F&x64.

Thanks,
Lili.