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

[Jan Beulich <jbeulich@suse.com>]

(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?

> +{
> +  /* 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.

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.

> @@ -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.

Jan