[PATCH 1/2] Add check for 8-bit old registers in EVEX format

[Jan Beulich]

On 15.05.2024 08:31, Cui, Lili wrote:
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -7029,6 +7029,18 @@ md_assemble (char *line)
>  	  as_bad (_("{rex2} prefix invalid with `%s'"), insn_name (&i.tm));
>  	  return;
>  	}
> +      /* Check for 8 bit operand that uses old registers.  */
> +      for (unsigned int op = 0; op < i.operands; op++)
> +	{
> +	  if (i.types[op].bitfield.class == Reg
> +	      && i.types[op].bitfield.byte
> +	      && !(i.op[op].regs->reg_flags & RegRex64)
> +	      && i.op[op].regs->reg_num > 3)
> +
> +	    as_bad (_("can't encode register '%s' in an "
> +		      " EVEX/VEX prefix instruction"),
> +		    i.op[op].regs->reg_name);
> +	}

I'd like us to avoid duplicating the conditional used here, matching what
establish_rex() does. establish_rex() is called unconditionally, so there
are two questions (both of which could have been addressed if there wasn't,
once again, an entirely empty patch description here): Why does the
checking there not cover this case? Is it not possible to amend that logic,
rather than introducing another instance in an entirely distinct place?

Also, nit: If this can't be folded with the existing checking, please
don't separate the as_bad() invocation by a blank line from its controlling
if(). This makes the two appear as if they don't belong together.

> --- a/gas/testsuite/gas/i386/x86-64-apx-inval.l
> +++ b/gas/testsuite/gas/i386/x86-64-apx-inval.l
> @@ -12,3 +12,6 @@
>  .*:13: Error: \{nf\} unsupported for `mulx'
>  .*:14: Error: \{nf\} cannot be combined with \{vex\}/\{vex3\}
>  .*:15: Error: \{nf\} cannot be combined with \{vex\}/\{vex3\}
> +.*:16: Error: can't encode register 'ah' in an  EVEX/VEX prefix instruction
> +.*:17: Error: can't encode register 'ah' in an  EVEX/VEX prefix instruction

Nit: You did observe the odd double blank in the diagnostic, didn't you?

Plus: Why the mention of VEX? There isn't any VEX-encoded insn permitting
8- or 16-bit register operands, is there? (Otherwise I don't see why XOP
wouldn't need mentioning, too.)

Plus: The register wants presenting differently - see establish_rex().

> --- a/gas/testsuite/gas/i386/x86-64-apx-inval.s
> +++ b/gas/testsuite/gas/i386/x86-64-apx-inval.s
> @@ -13,3 +13,5 @@
>  	{nf} mulx %r15,%r15,%r11
>  	{nf} {vex} bextr %ecx, %edx, %r10d
>  	{vex} {nf} bextr %ecx, %edx, %r10d
> +        {nf} add %dl,%ah
> +        {evex} adc %dl,%ah

Nit: Inconsistent indentation.

Jan