[PATCH] [AArch64] Make FP instructions optional

[Bernard Ogden]

Hi Tim,

I've been thinking a lot about arch combinations lately, albeit in the context of AArch32 where the world is less clean.

I tend to think its better to keep subtarget features independent, as it gives more future flexibility in the face of (1) future changes in the architecture/differences in other profiles of the architecture and (2) customer use cases. Is there enough advantage in NEON-implies-FP to outweigh the loss of flexibility?

If you really want to model the architecture here then don't you need a single feature controlling both FP and NEON? The ARM-ARM today requires both or neither.

But I agree with you about defaults. We should assume FP & NEON are available (and crypto, in my opinion, but that's a different discussion).

Regards,

Bernie

> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> bounces at cs.uiuc.edu] On Behalf Of Tim Northover
> Sent: 29 October 2013 14:24
> To: t.p.northover at gmail.com; Amara Emerson
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [PATCH] [AArch64] Make FP instructions optional
>
>
>   Hi Amara,
>
>   Is there a specification for this anywhere? It's a little unclear
> which instructions are allowed and not allowed. For example "ldr s0,
> [x0]" seems to be permitted even with -fp-armv8. Is that intentional?
>
>
> ================
> Comment at: lib/Target/AArch64/AArch64.td:27-28
> @@ -24,3 +26,4 @@
> +
>  def FeatureNEON : SubtargetFeature<"neon", "HasNEON", "true",
>    "Enable Advanced SIMD instructions">;
>
> ----------------
> Please tell me that HasNEON implies HasFPARMV8 (in which case this
> definition should say so).
>
> ================
> Comment at: test/MC/AArch64/fp-instructions.txt:1
> @@ +1,2 @@
> +# RUN: llvm-mc -triple=aarch64 -mattr=+fp-armv8 -disassemble < %s |
> FileCheck %s
> +
> ----------------
> This should be in test/MC/Disassembler/AArch64
>
> ================
> Comment at: test/CodeGen/AArch64/neon-bitwise-instructions.ll:1
> @@ -1,2 +1,2 @@
> -; RUN: llc < %s -verify-machineinstrs -mtriple=aarch64-none-linux-gnu
> -mattr=+neon | FileCheck %s
> +; RUN: llc < %s -verify-machineinstrs -mtriple=aarch64-none-linux-gnu
> -mattr=+neon,+fp-armv8 | FileCheck %s
>
> ----------------
> Why is this needed? I thought fp-armv8 was the default?
>
> ================
> Comment at: test/CodeGen/AArch64/alloca.ll:1-2
> @@ -1,2 +1,3 @@
> -; RUN: llc -mtriple=aarch64-none-linux-gnu -verify-machineinstrs < %s
> | FileCheck %s
> +; RUN: llc -mtriple=aarch64-none-linux-gnu -mattr=-fp-armv8 -verify-
> machineinstrs < %s | FileCheck %s
> +; RUN: llc -mtriple=aarch64-none-linux-gnu -mattr=+fp-armv8 -verify-
> machineinstrs < %s | FileCheck --check-prefix=FP %s
>
> ----------------
> In this (and all others) I strongly believe FP should be the default.
> Virtually everyone will be using a CPU with FP. Even if it was simply a
> matter of the name I'd be very unhappy, but I certainly want the
> default testing regime to be the common case.
>
> Similarly, we should have a very good explanation every time we need to
> specify "-mattr=+fp-armv8". Giving them equal status is not sufficient
> in my mind, because they don't have it.
>
> Also, if one prefix starts with CHECK, they probably both should.
>
> ================
> Comment at: test/CodeGen/AArch64/alloca.ll:92-93
> @@ +91,4 @@
> +
> +define void @test_variadic_alloca_nofp(i64 %n, ...) {
> +; CHECK-LABEL: test_variadic_alloca_nofp:
> +
> ----------------
> Isn't this identical to @test_variadic_alloca? If so you can just use
> these CHECK lines there too.
>
> ================
> Comment at: test/MC/AArch64/basic-a64-instructions.s:1698-1702
> @@ -1697,7 +1697,1 @@
>  //--------------------------------------------------------------------
> ----------
> -// Floating-point compare
> -//--------------------------------------------------------------------
> ----------
> -
> -        fcmp s3, s5
> -        fcmp s31, #0.0
> -// CHECK: fcmp    s3, s5                  // encoding:
> [0x60,0x20,0x25,0x1e]
> ----------------
> Similarly these are basic instructions. I don't think they need moving
> out of this file.
>
>
> http://llvm-reviews.chandlerc.com/D2052
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits


-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No:  2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No:  2548782