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

[Richard Barton]

Hi Bill

Committed as r155700

Thanks for reviewing.

Rich

> -----Original Message-----
> From: Bill Wendling [mailto:wendling at apple.com]
> Sent: 26 April 2012 21:07
> To: Richard Barton
> Cc: llvm-commits at cs.uiuc.edu; Jim Grosbach
> Subject: Re: [PATCH] Refactor MCInst operands for Thumb IT instruction
>
> 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>
>
>


-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.