[PATCH 04/10] Support Intel CMPccXADD

[Jan Beulich]

On 27.10.2022 05:09, Jiang, Haochen wrote:
> 
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Wednesday, October 26, 2022 4:50 PM
>> To: Jiang, Haochen <haochen.jiang@intel.com>
>> Cc: hjl.tools@gmail.com; binutils@sourceware.org
>> Subject: Re: [PATCH 04/10] Support Intel CMPccXADD
>>
>> On 26.10.2022 05:03, Jiang, Haochen wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: Tuesday, October 25, 2022 2:53 PM
>>>> To: Jiang, Haochen <haochen.jiang@intel.com>
>>>> Cc: hjl.tools@gmail.com; binutils@sourceware.org
>>>> Subject: Re: [PATCH 04/10] Support Intel CMPccXADD
>>>>
>>>> On 24.10.2022 07:55, Jiang, Haochen wrote:
>>>>>> -----Original Message-----
>>>>>> From: Jan Beulich <jbeulich@suse.com>
>>>>>> Sent: Friday, October 14, 2022 9:47 PM
>>>>>>
>>>>>> On 14.10.2022 11:12, Haochen Jiang wrote:
>>>>>> And finally - is SwapSources really appropriate to use here?
>>>>>> There's only
>>>> one
>>>>>> pure source operand, the other two are also serving as destinations.
>>>>>> I wonder whether an attribute is necessary here in the first place:
>>>>>> Vex-
>>>> encoded
>>>>>> insns with a memory destination never have two further register
>>>> operands, so
>>>>>> that property should suffice for identifying the case in
>>>> build_modrm_byte().
>>>>>> Alternatively you could also simply use the CPU flag.
>>>>>
>>>>> We may need a special identifier for CMPccXADD since we have VVVV at
>>>>> operand 3, where it is always at operand 2 for all other insts which
>>>>> have VVVV. That is the reason we reuse SwapSources. It might be not
>>>>> that same as the original meaning. But we want to avoid adding a bit
>>>>> for this very rare case. Do we need to change that?
>>>>
>>>> Re-using existing attributes is certainly preferred. But the question
>>>> here was whether _any_ special attribute is needed. Did you try out
>>>> my suggestion, and it didn't work out for some reason? Avoiding the
>>>> (ab)use of an inappropriately (for the purpose here) named attribute
>>>> would imo be preferable.
>>>
>>> Actually we have some similar instructions. For example, vmaskmovps/d.
>>> It could also take one memory operand as dest and two register
>>> operands as source.
>>
>> As you say there are two _source_ operands here, whereas ...
>>
>>> However, cmp<cc>xadd has a different encoding pattern with it. The
>>> default behavior for previous insts like vmaskmovps/d in Intel syntax
>>> is to encode first register operand as vvvv and second register operand as
>> modrm:reg.
>>> In cmp<cc>xadd, it is swapped, with first register operand as
>>> modrm:reg and second register operand as vvvv. I suppose it makes
>>> sense to use SwapSource and it is quite hard or might be dirty to just
>>> use the number of registers to identify them in build_modrm_byte().
>>
>> ... this has two destinations (memory and a register). Which is why using
>> SwapSource at best is a slight abuse (which, as said, I'd prefer to avoid).
>> But yes, I agree that going _just_ from a memory destination with two
>> further register ops is not sufficient.
>>
>> Seeing what H.J. and I are suggesting for the PREFETCHI change: How about
>> using the CPU flag in the conditional that you add?
> 
> That is ok for me. I suppose the cmpccxadd might be the special one for very
> long time. 
> 
> However, the potential problem is when the same scenario happen again. We
> will have to add another cpuid at that condition again, which is a little dirty.

I guess we can decide at that point whether a 2nd CPU check is preferable
over whatever alternative.

Jan