[PATCH] [AArch64] Make FP instructions optional

[Tim Northover]

  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