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

James Molloy james.molloy at arm.com
Tue Aug 16 09:38:06 PDT 2011 

Hi Jim,

The elaboration is that it appears Kristof didn't correctly update our
internal bug tracking system with his altered patch based on your last
review.

I've now taken the patch as it was last sent to the list and updated it to
change the #include and add tests as mentioned. I believe that the patch now
no longer requires any changes to clang either.

Apologies,

James

> -----Original Message-----
> From: Jim Grosbach [mailto:grosbach at apple.com]
> Sent: 16 August 2011 17:11
> To: James Molloy
> Cc: Kristof Beyls; llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH] Fix NOP encodings in ARM backend.
> 
> Hi James,
> 
> This patch switches back to adding the SubtargetInfo to the API outside
> of the ARM target. Can you elaborate a bit on why? If it's necessary,
> that's fine, I just don't want to change the interfaces if we don't
> need to.
> 
> -Jim
> 
> On Aug 16, 2011, at 8:42 AM, James Molloy wrote:
> 
> > 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.
> >>>
> >>
> >>
> > <nop.patch>
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: nop.v2.patch
Type: application/octet-stream
Size: 8064 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110816/9f5529cb/attachment.obj>

More information about the llvm-commits mailing list