[llvm-commits] [PATCH] Fix NOP encodings in ARM backend.
Jim Grosbach
grosbach at apple.com
Tue Aug 16 10:07:45 PDT 2011
Hi James,
No worries. I just wanted to make sure I wasn't missing something.
Applied in r137723.
Thanks again!
-Jim
On Aug 16, 2011, at 9:38 AM, James Molloy wrote:
> 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>
>>
>>
> <nop.v2.patch>
More information about the llvm-commits
mailing list