[PATCH] Support APX CFCMOV

[Cui, Lili]

> >> On 18.06.2024 11:24, Cui, Lili wrote:
> >>>> On 17.06.2024 08: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?

I will take AL for the next two days, email responses will be slower.

Thanks,
Lili.