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

[Jim Grosbach]

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>