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

Jim Grosbach grosbach at apple.com
Tue Aug 16 09:11:07 PDT 2011 

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>

More information about the llvm-commits mailing list