[rfc/patch][pr 22995] Swapped register order of "cmp" instructions

[Ahmed Bougacha]

As Craig says, you can just remove the argument altogether, as
MRMDestReg is the default.
With that, LGTM.

Thanks!

-Ahmed


On Mon, Mar 30, 2015 at 5:42 PM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
> Ah, much better, thanks!
>
> An updated patch is attached.
>
> On 30 March 2015 at 19:50, Craig Topper <craig.topper at gmail.com> wrote:
>> I agree these lines need the MRMSrcReg argument removed.
>>
>>       def TEST8rr  : BinOpRR_F<0x84, "test", Xi8 , X86testpat, MRMSrcReg>;
>>       def TEST16rr : BinOpRR_F<0x84, "test", Xi16, X86testpat, MRMSrcReg>;
>>       def TEST32rr : BinOpRR_F<0x84, "test", Xi32, X86testpat, MRMSrcReg>;
>>       def TEST64rr : BinOpRR_F<0x84, "test", Xi64, X86testpat, MRMSrcReg>;
>>
>> On Mon, Mar 30, 2015 at 3:31 PM, Ahmed Bougacha <ahmed.bougacha at gmail.com>
>> wrote:
>>>
>>> I was talking about CMP, but the PR is about TEST only, right?
>>>
>>> If so, I think the right fix is to use the default MRMDestReg Format
>>> for TEST*rr, like we do for the other MR-encoded instructions.  Craig,
>>> does that sound right to you?
>>>
>>> On Mon, Mar 30, 2015 at 1:32 PM, Rafael Espíndola
>>> <rafael.espindola at gmail.com> wrote:
>>> >> I can't say anything about reasons, but this doesn't seem like a
>>> >> printing issue, rather one of default encoding?  There's 38/39 for
>>> >> "r/m,
>>> >> r", and 3A/3B for "r, r/m":  I guess MC and GAS don't agree on that.
>>> >
>>> > The encoding is different, but the instructions are symmetrical.
>>>
>>> Disregard, that doesn't apply to TEST, which only has one opcode.
>>> Sorry about the noise!
>>>
>>> Back to the actual issue:  I think MC is wrong. In the SDM, looking at
>>> the RM encoding of TEST:  it takes op1 = ModRM:r/m, and op2 =
>>> ModRM:reg.
>>>
>>> In AT&T syntax, for:
>>>
>>>   test %ebx, %eax
>>>
>>> The ModRM byte should be 0xd8 (2.1.5, table 2-2).   Feeding that to MC
>>> gives us 0xc3 instead:
>>>
>>>   testl %ebx, %eax              ## encoding: [0x85,0xc3]
>>>                                           ## <MCInst #2894 TEST32rr
>>>                                           ##  <MCOperand Reg:19>
>>>                                           ##  <MCOperand Reg:21>>
>>>
>>> However, disassembling via MC the MR encoding for CMP, which is 0x39
>>> 0xd8, we get:
>>>
>>>   cmpl %ebx, %eax              ## encoding: [0x39,0xd8]
>>>                                           ## <MCInst #540 CMP32rr
>>>                                           ##  <MCOperand Reg:19>
>>>                                           ##  <MCOperand Reg:21>>
>>>
>>> Tying it back to the .td, we defined TEST*rr with the MRMSrcReg
>>> format, and CMP with MRMDestReg, and this is the root problem.
>>> Grepping around for MRMSrcReg, we use it for the RM encoding: IMUL,
>>> ADCX, and the various "r, r/m" _REV instructions.  The MRMDestReg
>>> format is for the MR encoding, and should be used for TEST.
>>>
>>> -Ahmed
>>
>>
>>
>>
>> --
>> ~Craig