[PATCH] Support APX CFCMOV

[Jan Beulich]

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.

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

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

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

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

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

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

Jan