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

James Molloy james.molloy at arm.com
Tue Aug 16 08:59:02 PDT 2011 

Hi,

Sorry, this clang patch, identical to that attached to the original issue,
is also required.

James

> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> bounces at cs.uiuc.edu] On Behalf Of James Molloy
> Sent: 16 August 2011 16:42
> To: 'Jim Grosbach'
> Cc: Kristof Beyls; llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH] Fix NOP encodings in ARM backend.
> 
> 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_clang.patch
Type: application/octet-stream
Size: 1155 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110816/198a5623/attachment.obj>

More information about the cfe-commits mailing list