[PATCH] Support APX CFCMOV

[Cui, Lili]

> On 17.06.2024 08:18, Cui, Lili wrote:
> > --- /dev/null
> > +++ b/gas/testsuite/gas/i386/x86-64-apx-cfcmov.s
> > @@ -0,0 +1,680 @@
> > +# Check 64bit EVEX-promoted CMOVcc instructions
> > +
> > +	.text
> > +_start:
> > +	cfcmovb	%dx,%ax
> 
> What about cmova or cmovae? Others look to not be covered either. Every
> condition code mnemonic suffix should imo be tested at least once.

For the assembler, I used <cc> as recommended by spec, cmova = comvnbe/ cmovae = cmovnb
For the disassembler, I used the same format as jcc.

> 
> > +	cfcmovb.s	%dx,%ax
> > +	cfcmovb	%dx,%ax,%r31w
> > +	cfcmovb	%dx,291(%r8,%rax,4)
> > +	cfcmovb	%ecx,%edx
> > +	cfcmovb	%ecx,%edx,%r10d
> > +	cfcmovb	%ecx,291(%r8,%rax,4)
> > +	cfcmovb	%r31,%r15
> > +	cfcmovb	%r31,%r15,%r11
> > +	cfcmovb	%r31,291(%r8,%rax,4)
> > +	cfcmovb	291(%r8,%rax,4),%dx
> > +	cfcmovb	291(%r8,%rax,4),%dx,%ax
> > +	cfcmovb	291(%r8,%rax,4),%ecx
> > +	cfcmovb	291(%r8,%rax,4),%ecx,%edx
> > +	cfcmovb	291(%r8,%rax,4),%r31
> > +	cfcmovb	291(%r8,%rax,4),%r31,%r15
> > +	cfcmovbe	%dx,%ax
> > +	cfcmovbe	%dx,%ax,%r31w
> > +	cfcmovbe	%dx,291(%r8,%rax,4)
> > +	cfcmovbe	%ecx,%edx
> > +	cfcmovbe	%ecx,%edx,%r10d
> > +	cfcmovbe	%ecx,291(%r8,%rax,4)
> > +	cfcmovbe	%r31,%r15
> > +	cfcmovbe	%r31,%r15,%r11
> > +	cfcmovbe	%r31,291(%r8,%rax,4)
> > +	cfcmovbe	291(%r8,%rax,4),%dx
> > +	cfcmovbe	291(%r8,%rax,4),%dx,%ax
> > +	cfcmovbe	291(%r8,%rax,4),%ecx
> > +	cfcmovbe	291(%r8,%rax,4),%ecx,%edx
> > +	cfcmovbe	291(%r8,%rax,4),%r31
> > +	cfcmovbe	291(%r8,%rax,4),%r31,%r15
> 
> If I'm not mistaken, operands are all the same between the condition code
> groups. In such a case, did you consider using .irp to reduce source volume
> (and redundancy) quite a bit?
> 

Yes, good idea.

> > +	cmovb	%dx,%ax,%r31w
> > +	cmovb	%ecx,%edx,%r10d
> 
> Don't we have cmov<cc> tests already? I don't mind having more, but in such
> a case that's once again something to mention in the description.
> 

Ok, I'll add descriptions in the changelog.

> > @@ -10565,7 +10567,17 @@ putop (instr_info *ins, const char
> *in_template, int sizeflag)
> >  		}
> >  	    }
> >  	  else if (l == 1 && last[0] == 'C')
> > -	    break;
> > +	    {
> > +	      if (ins->vex.nd && !ins->vex.nf)
> > +		break;
> > +	      else
> 
> Nit: This is again one of these odd "else". Without it code, beyond being a tiny
> bit shorter, ...
> 
> > +		{
> > +		  *ins->obufp++ = 'c';
> > +		  *ins->obufp++ = 'f';
> > +		  /* Skip printing {evex} */
> > +		  evex_printed = true;
> > +		}
> 
> ... will also get away with less indentation (and no braces) here.
> 

Done.

> > @@ -14046,3 +14058,39 @@ JMPABS_Fixup (instr_info *ins, int bytemode,
> int sizeflag)
> >      return OP_IMREG (ins, bytemode, sizeflag);
> >    return OP_OFF64 (ins, bytemode, sizeflag);  }
> > +
> > +static bool
> > +CFCMOV_Fixup_op0 (instr_info *ins, int bytemode, int sizeflag) {
> > +  /* EVEX.NF is used as a direction bit in the 2-operand case to reverse the
> > +     source and destination operands. */
> > +  if (!ins->vex.nd && ins->vex.nf)
> > +    {
> > +      bytemode = v_swap_mode;
> 
> Why is this? Iirc *_swap_mode are used when alternative encodings for the
> same operand combinations exist, which wouldn't be recognizable. They also
> look to only take effect in suffix-always mode, yet you don't add any respective
> testing. (Which isn't to mean I think you should add such. But if this line above
> was really meaningful, then such testing would be needed, to show its effects
> and to ensure those effects won't go away going forward.)
> 

I enabled "-Msuffix" in x86-64-apx-cfcmov-intel.d, unfortunately, it wasn't obvious where I put it. I will add more tests and put it at the end of the tests.
 
[       ]*[a-f0-9]+:[   ]*62 f4 7d 0c 42 d0[    ]+cfcmovb.s ax,dx

> > +      return OP_E (ins, bytemode, sizeflag);
> > +    }
> > +  else
> > +    {
> > +      return OP_G (ins, bytemode, sizeflag);
> > +    }
> > +}
> > +
> > +static bool
> > +CFCMOV_Fixup_op1 (instr_info *ins, int bytemode, int sizeflag) {
> > +  /* EVEX.NF is used as a direction bit in the 2-operand case to reverse the
> > +     source and destination operands. */
> > +  if (!ins->vex.nd && ins->vex.nf)
> > +    {
> > +      /* These bits have been consumed and should be cleared.  */
> > +      ins->vex.nf = false;
> > +      ins->vex.mask_register_specifier = 0;
> > +      return OP_G (ins, bytemode, sizeflag);
> > +    }
> > +  else
> > +    {
> > +      ins->vex.nf = false;
> > +      ins->vex.mask_register_specifier = 0;
> 
> It's certainly a little odd for this to be done identically in if and else.
> 

Done.

> > +      return OP_E (ins, bytemode, sizeflag);
> > +    }
> > +}
> 
> I wonder anyway whether we actually need two functions here. The mode put
> in the table entries is only ever v_mode. By slightly abusing that, you could use
> that parameter to indicate which operand it is, then passing v_mode explicitly
> to OP_E() / OP_G(). We apparently have a precedent for this already:
> NOP_Fixup() is quite similar in this regard. A few others also don't really have
> a mode passed, but they're somewhat different in nature.
> 
It is a good example, changed.

Thanks,
Lili.