Hi, Sorry, this clang patch, identical to that attached to the original issue, is also required.
James > -----Original Message----- > From: [email protected] [mailto:llvm-commits- > [email protected]] On Behalf Of James Molloy > Sent: 16 August 2011 16:42 > To: 'Jim Grosbach' > Cc: Kristof Beyls; [email protected] > 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:[email protected]] > > Sent: 15 August 2011 19:27 > > To: James Molloy > > Cc: Kristof Beyls; [email protected] > > 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" <[email protected]> > 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:[email protected]] > > >>> Sent: 10 August 2011 00:08 > > >>> To: Kristof Beyls > > >>> Cc: [email protected]; [email protected] > > >>> 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 > > >>>> [email protected] > > >>>> 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_clang.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
