[PATCH] Support APX CFCMOV

[Jan Beulich]

On 19.06.2024 11:18, Cui, Lili wrote:
>>>>>>>>> --- a/opcodes/i386-opc.tbl
>>>>>>>>> +++ b/opcodes/i386-opc.tbl
>>>>>>>>> @@ -989,6 +989,10 @@ ud0, 0xfff, i186,
>>>>>>>>> Modrm|CheckOperandSize|No_bSuf|No_sSuf,
>>>> { Reg16|Reg32|Reg64|Un
>>>>>>>>> cmov<cc>, 0x4<cc:opc>, CMOV&APX_F,
>>>>>>>>> Modrm|CheckOperandSize|No_bSuf|No_sSuf|DstVVVV|EVexMap4, {
>>>>>>>>> Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64,
>>>>>>>>> Reg16|Reg32|Reg64 }  cmov<cc>, 0xf4<cc:opc>, CMOV,
>>>>>>>>> Modrm|CheckOperandSize|No_bSuf|No_sSuf, {
>>>>>>>>> Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }
>>>>>>>>>
>>>>>>>>> +cfcmov<cc>, 0x4<cc:opc>, CMOV&APX_F,
>>>>>>>>>
>>>> +Modrm|CheckOperandSize|No_bSuf|No_sSuf|DstVVVV|EVexMap4|NF, {
>>>>>>>>> +Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64,
>>>>>>>>> +Reg16|Reg32|Reg64 } cfcmov<cc>, 0x4<cc:opc>, CMOV&APX_F,
>>>>>>>>> +Load|Modrm|CheckOperandSize|No_bSuf|No_sSuf|EVexMap4, {
>>>>>>>>> +Reg16|Reg32|Reg64|Unspecified|BaseIndex,
>> Reg16|Reg32|Reg64 }
>>>>>>>>> +cfcmov<cc>, 0x4<cc:opc>, CMOV&APX_F,
>>>>>>>>> +Modrm|CheckOperandSize|No_bSuf|No_sSuf|EVexMap4|NF, {
>>>>>>>>> +Reg16|Reg32|Reg64,
>> Reg16|Reg32|Reg64|Unspecified|BaseIndex }
>>>>>>>>
>>>>>>>> Just on this one for starters (I'll get to the other parts later):
>>>>>>>> NF here permits use of {nf}, which is specifically not wanted.
>>>>>>>> EVEX.NF has a different purpose here, and hence the pseudo
>>>>>>>> prefix, if used, needs to
>>>>>> be rejected.
>>>>>>>
>>>>>>> Added.
>>>>>>
>>>>>> To perhaps save a round trip on the eventual v2, would you mind
>>>>>> clarifying what you mean by "added"? The point of my remark was
>>>>>> that I think NF here needs dropping, with the setting of EVEX.NF in
>>>>>> the resulting encoding being driven by other means. Yet of course
>>>>>> the overall intended effect could also be achieved by keeping the
>>>>>> attribute here, and adding checks elsewhere (which is what I'd like
>>>>>> to
>>>> avoid).
>>>>>>
>>>>>
>>>>> I added 3 invalid test cases for it, and changed the code in a way
>>>>> you didn't
>>>> like. I thought it was a relatively minor change. I think the
>>>> advantage of this approach is that the changes were relatively minor.
>>>>>
>>>>>        /* Check NF support.  */
>>>>>        specific_error = progress (unsupported_nf);
>>>>> -      if (i.has_nf && !t->opcode_modifier.nf)
>>>>> +      if (i.has_nf && (!t->opcode_modifier.nf || is_cpu (t,
>>>>> + CpuCMOV)))
>>>>>         continue;
>>>>
>>>> With the presence of the CMOV feature bit being under question
>>>> anyway, I find this particularly odd. I also don't think the
>>>> alternative approach that I suggested would be meaningfully more
>> intrusive.
>>>
>>> Internal discussion has not started yet, but I personally think that we are
>> used to putting r/m at the end because it is more in line with the actual coding
>> order. Of course, your point of view is more humane.
>>>
>>>> However, in the end it's not
>>>> so much the tc-i386.c change I'm concerned about. It's rather the NF
>>>> attribute in the opcode table which I consider wrong to have for these
>> insns.
>>>>
>>>
>>> I understand your concern, just like the NDD bit is reused, the spec gives an
>> alias ZU, the NF reuse SPEC does not give an alias. I think it maybe because it is
>> only used to distinguish instructions from the encoding and does not have
>> much practical meaning. Of course, we can also add a new attribute for it in
>> OperandConstraint. But I don't have any good ideas for the new attribute
>> name. Do you have some suggestions?
>>
>> While being an option, I don't think anything like that is needed. The need to
>> set EVEX.NF can be recognized in a way similar to whatever you have done /
>> would do in the code snippet still visible above. I.e.
>> special-case these two forms by checking some specific property of theirs; if
>> nothing else, their opcodes. Only if that check becomes too complex /
>> expensive, going the OperandConstraint route would likely be the better route.
>> Plus, of course, unless you're aware of future additions to the ISA which
>> similarly re-purpose EVEX.NF.
>>
> 
> I think for these two insn templates, their opcodes are the same, and we can't distinguish them when both operands are regs (if we want to remove the attributes from the second one).
> 
> cfcmov<cc>, 0x4<cc:opc>, CMOV&APX_F, Load|Modrm|CheckOperandSize|No_bSuf|No_sSuf|EVexMap4, { Reg16|Reg32|Reg64|Unspecified|BaseIndex, Reg16|Reg32|Reg64 }
> cfcmov<cc>, 0x4<cc:opc>, CMOV&APX_F, Modrm|CheckOperandSize|No_bSuf|No_sSuf|EVexMap4|NF, { Reg16|Reg32|Reg64, Reg16|Reg32|Reg64|Unspecified|BaseIndex }

These two can be told from one another by their operands (actual operands
don't matter here, it's what the templates permit that matters). But I can
see your point, especially when considering the other two templates as
well (the 3-operand CFCMOVcc and CMOVcc ones). There, except for the
mnemonics, we'd have nothing to tell them apart. And going by mnemonics is
prohibitive here, I think.

So yes, a new operand constraint it is then, perhaps EVexNF / EVEX_NF.

Jan