[llvm-commits] [PATCH] Fix NOP encodings in ARM backend.

James Molloy james.molloy at arm.com
Tue Aug 16 08:42:22 PDT 2011 

Hi Jim,

Attached is the modified patch, which adds 4 new MC tests testing for
thumb1, thumb2, arm pre 6t2 and arm post 6t2.

Cheers,

James

> -----Original Message-----
> From: Jim Grosbach [mailto:grosbach at apple.com]
> Sent: 15 August 2011 19:27
> To: James Molloy
> Cc: Kristof Beyls; llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH] Fix NOP encodings in ARM backend.
> 
> 
> On Aug 15, 2011, at 11:08 AM, James Molloy wrote:
> 
> > Hi Jim,
> >
> > Kristof is out of office for a few weeks, so I'll be taking over
> dealing with this patch.
> >
> > I'll change the code to include MCSubtargetInfo.h as you suggest. I'm
> not sure about what to do with testcases, because Kristof apparently
> found broken behaviour in another part of the MC (for which he's added
> a FIXME) that will stop a normal test from exhibiting the correct
> behaviour - see the first email in the chain for a better description
> of that.
> 
> Ah, right. Forgot about that part. Fixing the mode setting bit
> shouldn't be very invasive, I would hope. I haven't looked at that in
> detail recently, though, I confess.
> 
> In any case, I don't think that will prevent testcases. It just makes
> it not work to have them all be in a single source file (which we
> probably don't want anyway. One file for thumb and one file for ARM
> seems cleaner).
> 
> For example, we can get thumb padding with something like:
> $ cat x.s
> .thumb_func _foo
> .code 16
> _foo:
>         mov r0, r1
>         .align 4
>         mov r0, r2
> $ llvm-mc -triple=armv4-apple-darwin x.s -filetype=obj -o x.o
> $ otool -vt x.o
> x.o:
> (__TEXT,__text) section
> _foo:
> 00000000	    4608	mov	r0, r1
> 00000002	    bf00	nop
> 00000004	    bf00	nop
> 00000006	    bf00	nop
> 00000008	    bf00	nop
> 0000000a	    bf00	nop
> 0000000c	    bf00	nop
> 0000000e	    bf00	nop
> 00000010	    4610	mov	r0, r2
> 
> And ARM padding with something like:
> $ cat x.s
> _foo:
>         mov r0, r1
>         .align 4
>         mov r0, r2
> $ llvm-mc -triple=armv4-apple-darwin x.s -filetype=obj -o x.o
> $ otool -vt x.o
> x.o:
> (__TEXT,__text) section
> _foo:
> 00000000	e1a00001	mov	r0, r1
> 00000004	e1a00000	nop			(mov r0,r0)
> 00000008	e1a00000	nop			(mov r0,r0)
> 0000000c	e1a00000	nop			(mov r0,r0)
> 00000010	e1a00002	mov	r0, r2
> 
> The tests would want to use macho-dump, not otool, of course, for
> platform independence.
> 
> -Jim
> 
> 
> 
> > He was loath to have his first patch to the list be a large and
> contentious one, as he thinks it will be if he fixes the other
> brokenness immediately. What do you suggest?
> >
> > My suggestion would be to add tests but mark them XFAIL for now -
> would this be acceptable?
> >
> > Cheers,
> >
> > James
> >
> >
> >
> > On 15 Aug 2011, at 18:47, "Jim Grosbach" <grosbach at apple.com> wrote:
> >
> >> Hi Kristof,
> >>
> >> This is a lot closer. Using Target/TargetSubtargetInfo.h is a
> layering violation, however. Nothing in the MC layer, which includes
> the AsmBackend, should reference anything in the Target layer. In this
> case, just include MC/MCSubtargetInfo.h directly, instead.
> >>
> >> Also, please add test cases to the LLVM MC tests (in test/MC/ARM) to
> verify that this is doing what you expect. If you're not already
> familiar with it, there are some examples in there (prefetch.ll is a
> good one) for how to use the -check-prefix option FileCheck for this
> sort of conditional behaviour.
> >>
> >> Thanks again!
> >>
> >> -Jim
> >>
> >>
> >> On Aug 10, 2011, at 6:41 AM, Kristof Beyls wrote:
> >>
> >>> Hi Jim,
> >>>
> >>> Thanks to have a look!
> >>>
> >>> 1.  I need the exact functionality provided by
> ARMSubTarget::HasV6T2Ops.
> >>> Therefore, in trying not to reinvent the wheel, and
> >>> to avoid code duplication, I'm trying to use
> ARMSubTarget::HasV6T2Ops. I
> >>> think I've found a way to create an ARMSubTarget object from the
> Triple, so
> >>> that it doesn't have to be passed across a lot of interfaces. The
> patch now
> >>> only changes lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp (see
> attachment).
> >>> Since it no longer touches Clang, I dropped the cfe-commits list.
> Could you
> >>> have another look and see if this patch is closer to being
> acceptable?
> >>>
> >>> 2. I've created the following bug report:
> >>> http://llvm.org/bugs/show_bug.cgi?id=10632
> >>>
> >>>
> >>> Thanks,
> >>>
> >>> Kristof
> >>>
> >>> -----Original Message-----
> >>> From: Jim Grosbach [mailto:grosbach at apple.com]
> >>> Sent: 10 August 2011 00:08
> >>> To: Kristof Beyls
> >>> Cc: llvm-commits at cs.uiuc.edu; cfe-commits at cs.uiuc.edu
> >>> Subject: Re: [llvm-commits] [PATCH] Fix NOP encodings in ARM
> backend.
> >>>
> >>> Hi Kristof,
> >>>
> >>> Thanks for looking at this.
> >>>
> >>> 1. You should be able to derive the needed information from the
> Triple,
> >>> which is already passed in. There's already some code there that
> does
> >>> something similar to set the CPU Subtype correctly for Darwin MachO
> files.
> >>> See the factory method createARMAsmBackend() for details. There
> shouldn't be
> >>> any need to change the top level constructors or the target-
> independent
> >>> bits.
> >>>
> >>> 2. That sounds like a nasty bug. A bugzilla with a test case would
> be great.
> >>>
> >>> -Jim
> >>>
> >>> On Aug 8, 2011, at 6:54 AM, Kristof Beyls wrote:
> >>>
> >>>> Hi,
> >>>>
> >>>> With the attached patch, I'm trying to fix a FIXME in the ARM
> backend.
> >>> This
> >>>> patch fixes ARMAsmBackend::WriteNopData, so that it takes into
> account the
> >>>> version of the ARM architecture that is being targeted. For
> versions
> >>> before
> >>>> ARMv6T2, there is no NOP instruction, and NOPs are encoded as MOV
> r0,r0
> >>> (in
> >>>> ARM
> >>>> mode) or MOV r8,r8 (in Thumb mode). For targets later than
> ARMv6T2, the
> >>>> encoding for the NOP instruction is created.
> >>>>
> >>>> I have a few questions about this patch:
> >>>>
> >>>> 1. To make sure that ARMAsmBackend::WriteNopData can figure out
> which ARM
> >>>> sub-target it compiles for, I had to adapt the
> >>> Target::MCAsmBackendCtorTy
> >>>> to
> >>>> also pass on an MCSubtargetInfo argument. Is this the best way to
> get
> >>>> sub-target information to the ARMAsmBackend object?
> >>>> (this change results in a few function signature changes in the
> >>>> ARM, PowerPC, X86 and MBlaze backends).
> >>>>
> >>>> 2. It's hard to create test cases to test this properly, since I
> think
> >>>> that there is another bug in lib/MC/MCAssembler.cpp, where
> processing
> >>>> an alignment fragment results in calling
> ARMAsmBackend::WriteNopData,
> >>> but
> >>>> without putting the ARMAsmBackend in the right ARM or Thumb state.
> >>>> Therefore, e.g. when processing an assembler file with .align
> directives
> >>>> in the middle of a Thumb code section, still ARM NOP encodings are
> >>>> generated instead of Thumb NOP encodings.
> >>>> Question 2a: Is it OK to write a FIXME to indicate this
> brokenness?
> >>>> Should
> >>>> I also file a bugzilla issue?
> >>>> Question 2b: Is it OK to leave that fix for a later, separate,
> patch?
> >>> For
> >>>> that fix, it will be easier to create good test cases that will
> also
> >>> test
> >>>> this patch.
> >>>>
> >>>> Thanks,
> >>>>
> >>>> Kristof
> >>>>
> >>>> PS. I'm cc-ing to the cfe-commits list because the change in
> >>>> Target::MCAsmBackendCtorTy requires 2 lines to change in Clang
> too, see
> >>>> attached file
> >>>
> clang_arm_nop_encoding.patch.<llvm_arm_nop_encoding.patch><clang_arm_no
> p_enc
> >>> oding.patch>_______________________________________________
> >>>> llvm-commits mailing list
> >>>> llvm-commits at cs.uiuc.edu
> >>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>>
> >>>
> >>> <llvm_arm_nop_encoding_v2.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.
> >
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: nop.patch
Type: application/octet-stream
Size: 18666 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110816/7c96f551/attachment.obj>

More information about the llvm-commits mailing list