[PATCH] fix alignment specifier syntax on Neon instructions

[Kristof Beyls]

Thanks Bob, Jim & Renato.

I've committed the first patch in r175164.

For the second patch, it seems that it would be best for the ARMAsmPrinter
to 
use the correct syntax, except when targeting a darwin platform, where the
old
syntax should still be used?

I've had a look at how to be able to detect in the ARMAsmPrinter whether the
target is a Darwin platform or not. I couldn't see an elegant way to do it.
Out of all the slightly hacky approaches I could come up with, the best
option
to me seems to check the type of the MCAsmInfo object passed in to the 
ARMAsmPrinter constructor. If the type is ARMMCAsmInfoDarwin, we'd know
we're
targeting a darwin platform, and otherwise not.

To be able to do this, I think I first should implement LLVM-style RTTI for
the MCAsmInfo class hierarchy.

Does this sound like a reasonable approach, or would there be a better way
to be able to find out in ARMAsmPrinter whether it's targeting a Darwin
platform or not?

Thanks,

Kristof

> -----Original Message-----
> From: Bob Wilson [mailto:bob.wilson at apple.com]
> Sent: 13 February 2013 18:02
> To: Jim Grosbach
> Cc: Kristof Beyls; llvm-commits at cs.uiuc.edu; Evan Cheng
> Subject: Re: [PATCH] fix alignment specifier syntax on Neon instructions
> 
> 
> On Feb 12, 2013, at 2:34 PM, Jim Grosbach <grosbach at apple.com> wrote:
> 
> > Hi Kristof,
> >
> > You're correct that the cctools Darwin 'as' does not accept the syntax
sans-
> comma. Newer toolchains, however, rely on LLVM's integrated assembler as
the
> system assembler on Darwin for both x86 and ARM.
> >
> > Conceptually, I really like this change, as it's bringing the toolchain
more
> into conformance with the published syntax. You're right that we need to
tread
> carefully to make sure current users are not disrupted by the transition.
I've
> added Bob and Evan for additional insight on that. I feel it'll be OK, as
the
> proposed patch continues to accept the old syntax, but would like their
buy-in
> before proceeding.
> >
> > -Jim
> 
> I have no objections to the first patch, to accept the new syntax.
> 
> I'm not so sure about the second.  I think we still have some users of the
old
> Apple cctools assembler that will break when using -no-integrated-as,
right?
> 
> >
> > On Feb 12, 2013, at 7:56 AM, Kristof Beyls <kristof.beyls at arm.com>
wrote:
> >
> >> Hi,
> >>
> >> The attached 2 patches fix the alignment specifier syntax on Neon
> >> instructions.
> >> The background to these patches is that currently, LLVM expects the
> >> alignment specifier in some Neon instructions to be specified using ",
:"
> >> syntax, e.g.
> >> 	vld1.8	{d16}, [r0, :64]
> >> However, the correct syntax is to use ":" instead of ", :", e.g.
> >> 	vld1.8	{d16}, [r0:64]
> >>
> >> Old versions of the GNU assembler got this wrong in a similar way, but
this
> >> was fixed in GNU as 2.20.xx (I'm not sure which minor version of 2.20
this
> >> was fixed in first). I believe that the current implementation in LLVM
> aimed
> >> at being compatible with GNU as. Since this has been fixed in GNU as
since
> >> at least 2010, I think this should now also be fixed in LLVM.
> >>
> >> Please review the attached 2 patches.
> >>
> >> The first patch, accept_correct_alignment_specifier_syntax.patch, just
> makes
> >> ARMAsmParser accept both syntaxes (both ", :" and ":"). I think it's
> >> important to keep on accepting the old syntax, so assembly written
using
> the
> >> old syntax still get accepted. Most of the patch consists of changing
test
> >> cases to use the correct syntax + also adding a few tests to make sure
the
> >> old syntax still gets accepted.
> >>
> >> The second patch, print_correct_alignment_specifier_syntax.patch, might
be
> >> slightly more controversial. This patch makes the ARMInstPrinter
generate
> >> the correct syntax (":") instead of the old incorrect syntax (", :").
This
> >> means that assembly output produced by LLVM cannot be assembled by GNU
as
> >> older than 2.20 anymore, and also potentially with some versions of
darwin
> >> as. I'd assume that GNU as 2.20 has been released long enough ago that
this
> >> should be OK. Is there anyone who knows whether darwin as also accepts
the
> >> correct syntax now, and whether this change would be OK to make?
> >>
> >> Thanks,
> >>
> >>
>
Kristof<accept_correct_alignment_specifier_syntax.patch><print_correct_align
me
> nt_specifier_syntax.patch>
> >
> 
>