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

[Jim Grosbach]

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_encoding.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits