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

[Cui, Lili]

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

Agreed, when I added this patch, I first tried adding it with rex/rex2. But since that judgment of establish_rex() is tied to "i.rex", as you also suggested, I created a patch to detach the old register check from that branch and then extended it to check the EVEX prefixed instructions .

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

Changed.

> 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.)
> 
Dropped VEX.

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

Thanks,
Lili.