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

[Richard Barton]

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>
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: IT_refactoring_mask_operand.patch
Type: application/octet-stream
Size: 3289 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120426/da82b6d2/attachment.obj>