[PATCH V2] Support APX CFCMOV

[Jan Beulich]

On 01.07.2024 08:37, Cui, Lili wrote:
>> On 25.06.2024 11:56, Cui, Lili wrote:
>>> @@ -14039,3 +14047,26 @@ 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_op (instr_info *ins, int opnd, 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)
>>> +    {
>>> +      if (opnd == 0)
>>> +	return OP_E (ins, v_swap_mode, sizeflag);
>>
>> There's still no testing of this use of v_swap_mode afaics.
>>
> 
> I thought adding -Msuffix in x86-64-apx-cfcmov-intel.d and doing the following tests were the tests you wanted, now it seems that I'm missing something. Could you add more information? Thanks.
> 
> [       ]*[a-f0-9]+:[   ]*62 f4 7d 08 42 c2[    ]+cfcmovb ax,dx
> [       ]*[a-f0-9]+:[   ]*62 f4 7d 0c 42 d0[    ]+cfcmovb.s ax,dx
> [       ]*[a-f0-9]+:[   ]*62 f4 7d 08 42 c2[    ]+cfcmovb ax,dx
> [       ]*[a-f0-9]+:[   ]*62 f4 7d 0c 42 d0[    ]+cfcmovb.s ax,dx

Hmm, I overlooked that, first and foremost because I certainly did not
expect this to be in an Intel syntax test. The above is not really Intel
syntax, and -Msuffix also doesn't make a lot of sense together with
-Mintel. (Read: Eventually we may want to actually reject that
combination, or re-purpose it to make the "<xyz> PTR" printing optional
when operand size is unambiguous. Nevertheless I can also see the
desire to have a way to express the non-default encodings in Intel
syntax output.)

>>> +      /* These bits have been consumed and should be cleared.  */
>>> +      ins->vex.nf = false;
>>> +      ins->vex.mask_register_specifier = 0;
>>
>> Hmm, I thought I had asked for this and ...
>>
>>> +      return OP_G (ins, v_mode, sizeflag);
>>> +    }
>>> +
>>> +  if (opnd == 0)
>>> +    return OP_G (ins, v_mode, sizeflag);
>>> +  /* These bits have been consumed and should be cleared.  */
>>> + ins->vex.nf = false;  ins->vex.mask_register_specifier = 0;
>>
>> ... this state update to be folded; I may be misremembering though. Any future
>> updating shouldn't require touching two entirely identical places in the same
>> (small) function. Plus when taking the first OP_E() path, you fail to clear -
>>> vex.nf right now anyway (i.e. another reason to do it once uniformly).
>>
> 
> I think you mean to clear vex.nf and vex.mask_register_specifier in only one place.

Indeed.

> I checked the logic, I think we can't fold them, we want to clear vex.nf before returning the last operand, the last operand has two cases,
> 
> Case 1: (!ins->vex.nd && ins->vex.nf) is true, and vex.nf needs to be cleared after judgment.
> Case 2: ins->vex.nf is true, and vex.nf needs to be cleared.( I added a condition to clear it exactly)

I don't see what good that condition does. It only make the code bigger.

>    if (opnd == 0)
>      return OP_G (ins, v_mode, sizeflag);
> -  /* These bits have been consumed and should be cleared.  */
> -  ins->vex.nf = false;
> -  ins->vex.mask_register_specifier = 0;
> +  if (ins->vex.nf)
> +    {
> +      /* These bits have been consumed and should be cleared.  */
> +      ins->vex.nf = false;
> +      ins->vex.mask_register_specifier = 0;
> +    }
>    return OP_E (ins, v_mode, sizeflag);
>  }

static bool
CFCMOV_Fixup_op (instr_info *ins, int opnd, int sizeflag)
{
  /* EVEX.NF is used as a direction bit in the 2-operand case to reverse the
     source and destination operands.  */
  bool dstmem = !ins->vex.nd && ins->vex.nf;

  if (opnd == 0)
    {
      if (dstmem)
	return OP_E (ins, v_swap_mode, sizeflag);
      return OP_G (ins, v_mode, sizeflag);
    }

  /* These bits have been consumed and should be cleared.  */
  ins->vex.nf = false;
  ins->vex.mask_register_specifier = 0;

  if (dstmem)
    return OP_G (ins, v_mode, sizeflag);
  return OP_E (ins, v_mode, sizeflag);
}

Jan