[llvm-commits] [PATCH] Refactor MCInst operands for Thumb IT instruction

[Bill Wendling]

Hi Richard,

Thanks for the explanation. I'm kind of surprised that we put the information into two different places. The patch looks good to me.

Thanks!
-bw

On Apr 26, 2012, at 3:14 AM, Richard Barton <richard.barton at arm.com> wrote:

> Hi Bill	
> 
> The previous implementation stored both the condition and the mask as separate
> operands in the MCInst, but the mask operand was stored as the 4-bit mask and
> the bottom bit of the condition. This was purportedly to make the inst printer
> work, as the bottom bit of the condition is needed to decode the mask into t/e
> conditions in the instruction mnemonic. 
> 
> I don't see any reason to do this when the condition value is available to the
> inst printer by arithmetic on the operand index as in my patch. This approach is
> taken widely throughout that code, the only difference being that it is normally
> an addition on the operand index rather than a subtraction. This still
> implicitly relies on the operands being in a particular order, so I argue that
> it is much of a muchness, and should be fine.
> 
> You can see the result of my patch using llvm-mc. The current behaviour is:
> 
> 09:57:22:/work/ricbar01/llvm-oss> echo 'iteee ne' | ./build/bin/llvm-mc -triple
> thumbv7 -show-inst -show-encoding 
>        .section        __TEXT,__text,regular,pure_instructions
>        iteee   ne                      @ encoding: [0x11,0xbf]
>                                        @ <MCInst #2250 t2IT
>                                        @  <MCOperand Imm:1>
>                                        @  <MCOperand Imm:17>>
> 
> The 0-th operand is the condition code 0x1 = NE, the 1-st operand is the mask in
> bits 3-0, which is:
> 
> NOT condition[0] NOT condition[0] NOT condition[0] 1 = 0001
> 
> with the bottom bit of the condition code as the 4-th bit of the mask.
> 
> My patch produces:
> 
> 09:57:22:/work/ricbar01/llvm-oss> echo 'iteee ne' | ./build/bin/llvm-mc -triple
> thumbv7 -show-inst -show-encoding 
>        .section        __TEXT,__text,regular,pure_instructions
>        iteee   ne                      @ encoding: [0x11,0xbf]
>                                        @ <MCInst #2250 t2IT
>                                        @  <MCOperand Imm:1>
>                                        @  <MCOperand Imm:1>>
> 
> I have to offer another apology. I have realised another mistake in the patch in
> that I have changed the functionality for illegal IT instructions with a 0x0
> mask. Previously MC converted these to a mask value of 0x8, which is "it
> <condition>" (no extra t or e's.) I have a problem with this anyway, but that is
> another patch entirely (my unallocated NOP can o' worms from yesterday). The
> attached, updated patch is pure refactoring, and reinstates the old behaviour
> for mask values of 0x0.
> 
> My patches for IT are colliding quite a lot, and I'm going to have to try harder
> in future to ensure that my patches are clearer when submitting to the list.
> 
> Thanks
> Rich
> 
> 
>> -----Original Message-----
>> From: Bill Wendling [mailto:wendling at apple.com]
>> Sent: 26 April 2012 00:21
>> To: Richard Barton
>> Cc: llvm-commits at cs.uiuc.edu for LLVM; Jim Grosbach
>> Subject: Re: [PATCH] Refactor MCInst operands for Thumb IT instruction
>> 
>> Hi Richard,
>> 
>> I may need one of the ARM guys to chime in here. My only question is about how
>> you're getting the "Firstcond" value in ARMInstPrinter.cpp. You're doing this:
>> 
>> 	unsigned Firstcond = MI->getOperand(OpNum-1).getImm();
>> 
>> Does this mean that the Mask and Firstcond information are both in the operand
>> list? And that when we created the mask and combined the two? If so, that's
>> gross...
>> 
>> -bw
>> 
>> On Apr 24, 2012, at 4:45 AM, Richard Barton <richard.barton at arm.com> wrote:
>> 
>>> Hi Bill
>>> 
>>> I'm taking you up on your invitation for more patch reviews in the area of
>> IT
>>> instructions.
>>> 
>>> The attached patch removes the slightly hacky use of the mask operand for IT
>>> instructions. Currently, the IT Mask operand is stored as the 4 mask bits
>> and
>>> the bottom bit of the condition code field. My patch stores the mask as just
>> the
>>> actual mask operand only and fetches the condition code operand separately
>> in a
>>> similar way to printers for other instructions.
>>> 
>>> Please review.
>>> 
>>> Richard Barton
>>> 
>>> <IT_refactoring_mask_operand.patch>
>> 
>> 
> <IT_refactoring_mask_operand.patch>