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

[Cui, Lili]

> Subject: Re: [PATCH 1/8] Support APX GPR32 with rex2 prefix
> 
> (for now only comments on i386-gen.c changes)
> 
> On 02.11.2023 12:29, Cui, Lili wrote:
> > @@ -1008,10 +1012,35 @@ get_element_size (char **opnd, int lineno)
> >    return elem_size;
> >  }
> >
> > +static bool
> > +if_entry_needs_special_handle (const unsigned long long opcode, unsigned
> int space,
> > +			       const char *cpu_flags)
> 
> This function wants to be named after its purpose, e.g. rex2_disallowed() with
> its current return value arrangement. "needs special handling" is a term that
> might be okay now, but what if you gus come up with REX3 in a few years time
> which then again needs (a different kind of) special handling?
> 
Done.

> > +{
> > +  /* Prefixing XSAVE* and XRSTOR* instructions with REX2 triggers
> > +#UD.  */
> > +  if (strcmp (cpu_flags, "XSAVES") >= 0
> > +      || strcmp (cpu_flags, "XSAVEC") >= 0
> > +      || strcmp (cpu_flags, "Xsave") >= 0
> > +      || strcmp (cpu_flags, "Xsaveopt") >= 0
> > +      || !strcmp (cpu_flags, "3dnow")
> > +      || !strcmp (cpu_flags, "3dnowA"))
> > +    return true;
> > +
> > +  /* All opcodes listed map0 0x4*, 0x7*, 0xa* and map0 0x3*, 0x8*
> > +     are reserved under REX2 and triggers #UD when prefixed with REX2
> > +*/
> > +  if ((space == 0 && (opcode >> 4 == 0x4
> > +		      || opcode >> 4 == 0x7
> > +		      || opcode >> 4 == 0xA))
> 
> What about row 0xE? Plus in the comment the latter is map1, not (again)
> map0.
> 

Done, thanks.

> This also may be easier to express using 0x4490 and
> 
> > +      || (space == SPACE_0F && (opcode >> 4 == 0x3
> > +				|| opcode >> 4 == 0x8)))
> 
> ... 0x0108 as constants. Else I'd like to ask that switch() be used to kept this
> halfway readable.
> 

Changed it to

+  if (space == 0)
+    switch (opcode >> 4)
+      {
+      case 0x4:
+      case 0x7:
+      case 0xA:
+      case 0xE:
+       return true;
+      default:
+       return false;
+    }
+
+  if (space == SPACE_0F)
+    switch (opcode >> 4)
+      {
+      case 0x3:
+      case 0x8:
+       return true;
+      default:
+       return false;
+      }
+
> > @@ -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;
> > +
> >      }
> 
> The comment is one half of what's needed here. First, however, you want to
> say a word on what this is about.
> 
Added. 

Thanks,
Lili