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

[Kristof Beyls]

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm_arm_nop_encoding_v2.patch
Type: application/octet-stream
Size: 5100 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110810/0212da88/attachment.obj>