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

Jim Grosbach grosbach at apple.com
Mon Aug 15 11:27:10 PDT 2011 

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_nop_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.
>

More information about the llvm-commits mailing list