[PATCH] fix alignment specifier syntax on Neon instructions

[Jim Grosbach]

Bob,

I don't think -no-integrated-as is a significant concern here. That's used by folks who are still using the pre-unified asm syntax, but this change is for NEON instructions, which are only available on architectures defined after unified syntax was introduced. While there still may be some overlap, I think it's worth forging ahead and deprecating the old support. We're talking about folks mixing pre-unified syntax inline assembly and compiler-generated NEON code in the same translation unit. I don't think that's a very common thing. We already require the integrated assembler for ISA extensions that the old assembler doesn't support (AVX on X86, for example). I don't know that this is really all that different. I'd prefer to forge ahead if possible, even if it means requiring some folks to update their code.

-Jim

On Feb 14, 2013, at 9:41 AM, Kristof Beyls <kristof.beyls at arm.com> wrote:

> 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>
>>> 
>> 
>> 
> 
> 
>