Re: [PATCH] Implement aarch64 neon instructio​n class SIMD misc - LLVM

[Tim Northover]

  Hi Hao,

  Lots of instructions here! Bet this one took you a while. A few comments:


================
Comment at: include/llvm/IR/IntrinsicsAArch64.td:41-44
@@ +40,6 @@
+def int_aarch64_neon_frinta : Neon_1Arg_Intrinsic;
+def int_aarch64_neon_frintp : Neon_1Arg_Intrinsic;
+def int_aarch64_neon_frintm : Neon_1Arg_Intrinsic;
+def int_aarch64_neon_frintx : Neon_1Arg_Intrinsic;
+def int_aarch64_neon_frintz : Neon_1Arg_Intrinsic;
+def int_aarch64_neon_frinti : Neon_1Arg_Intrinsic;
----------------
I think some of these are redundant. LLVM already has floor/ceil/... intrinsics that duplicate these semantics. Fortunately I think we worked out which ones they were with the vector work so you can probably copy that mapping.

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:3809
@@ +3808,3 @@
+/// within each block of the vector is reversed.)
+static bool isVREVMask(ArrayRef<int> M, EVT VT, unsigned BlockSize) {
+  assert((BlockSize == 16 || BlockSize == 32 || BlockSize == 64) &&
----------------
The AArch64 instruction is "REV" not "VREV".

================
Comment at: lib/Target/AArch64/AArch64InstrNEON.td:4794
@@ -4768,3 +4793,3 @@
 def neon_uimm1_bare : Operand<i64>,
-                        ImmLeaf<i64, [{(void)Imm; return true;}]> {
+                        ImmLeaf<i64, [{return Imm < 2;}]> {
   let ParserMatchClass = neon_uimm1_asmoperand;
----------------
Nice! I've been cringing whenever I see these for a while now.

================
Comment at: lib/Target/AArch64/AArch64InstrNEON.td:6430
@@ +6429,3 @@
+def : Pat<(v16i8 (sub 
+            (v16i8 (Neon_movi (i32 0), (i32 14))),
+            (v16i8 VPR128:$Rn))),
----------------
Is "(Neon_movi (i32 0), (i32 14)" -0.0? If so, some kind of PatFrag for it might be helpful.

================
Comment at: lib/Target/AArch64/AArch64InstrNEON.td:6932-6933
@@ +6931,4 @@
+
+multiclass NeonI_2VMisc_Extend_Pattern<string prefix> {
+def : Pat<(v4f32 (int_arm_neon_vcvthf2fp (v4i16 VPR64:$Rn))),
+          (!cast<Instruction>(prefix # "4h4s") VPR64:$Rn)>;
----------------
Indentation seems completely random in this section: 2 -> 0 -> 4 chars in 3 consecutive multiclasses.

================
Comment at: lib/Target/AArch64/AArch64InstrNEON.td:767-772
@@ -762,2 +766,8 @@
 
+def fpz32Short : Operand<f32>,
+                 ComplexPattern<f32, 1, "SelectFPZeroOperand", [fpimm]> {
+  let ParserMatchClass = neon_uimm0_asmoperand;
+  let PrintMethod = "printFPZeroOperandShort";
+  let DecoderMethod = "DecodeFPZeroOperand";
+}
 
----------------
What an appalling inconsistency in the spec! That said, I don't think there's any need for a new PrintMethod, the default Operand printer should do the job fine.

================
Comment at: test/CodeGen/AArch64/neon-compare-instructions.ll:54
@@ -53,4 +53,3 @@
 ;CHECK: cmeq {{v[0-9]+}}.8b, {{v[0-9]+}}.8b, {{v[0-9]+}}.8b
-;CHECK-NEXT: movi {{v[0-9]+}}.8b, #0xff
-;CHECK-NEXT: eor {{v[0-9]+}}.8b, {{v[0-9]+}}.8b, {{v[0-9]+}}.8b
+;CHECK-NEXT: not {{v[0-9]+}}.8b, {{v[0-9]+}}.8b
 	%tmp3 = icmp ne <8 x i8> %A, %B;
----------------
Nice improvement!


http://llvm-reviews.chandlerc.com/D2118