[PATCH] D41792: [AArch64] Add ARMv8.2-A FP16 scalar intrinsics

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Thanks for working on this! 
Some comments inline.



================
Comment at: clang/include/clang/Basic/arm_fp16.td:19
+// The operations are subclasses of Operation providing a list of DAGs, the
+// last of which is the return value. 
+//
----------------
nit: trailing whitespace.


================
Comment at: clang/include/clang/Basic/arm_fp16.td:58
+class IInst<string n, string p, string t> : Inst<n, p, t, OP_NONE> {}
+
+// ARMv8.2-A FP16 intrinsics.
----------------
There's a little bit of duplication here: the definitions above are the same as in arm_neon.td. Would it be easy to share this, with e.g. an include?


================
Comment at: clang/include/clang/Basic/arm_fp16.td:79
+
+  // Rounding 
+  def FRINTZ_S64H : SInst<"vrnd", "ss", "Sh">;
----------------
trailing whitespace


================
Comment at: clang/include/clang/Basic/arm_fp16.td:88
+
+  // Conversion 
+  def SCALAR_SCVTFSH   :  SInst<"vcvth_f16", "Ys", "silUsUiUl">;
----------------
trailing whitespace


================
Comment at: clang/include/clang/Basic/arm_fp16.td:89
+  // Conversion 
+  def SCALAR_SCVTFSH   :  SInst<"vcvth_f16", "Ys", "silUsUiUl">;
+  def SCALAR_FCVTZSH   :  SInst<"vcvt_s16", "$s", "Sh">;
----------------
Nit: for the definitions below, indentation is sometimes a bit off. I.e. some defs have 1 space after the semicolon others have 2.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:4102
   NEONMAP1(vuqadds_s32, aarch64_neon_suqadd, Add1ArgType),
+  // FP16 scalar intrinisics go here.
+  NEONMAP1(vabdh_f16, aarch64_sisd_fabd, Add1ArgType),
----------------
Looks like  a few intrinsic descriptions are missing here. For example, the first 2-operand intrinsic vaddh_f16 is missing, but there are also more. Is this intentional, or might they have slipped through the cracks (or am I missing something)?


================
Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td:149
                 [IntrNoMem]>;
+
+  class AdvSIMD_1Arg_Intrinsic
----------------
This and the other changes in this file are changes to LLVM. Do we need these changes for this patch? It doesn't look like it.


================
Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td:360
   // Vector Absolute Value
-  def int_aarch64_neon_abs : AdvSIMD_1IntArg_Intrinsic;
+  //def int_aarch64_neon_abs : AdvSIMD_1IntArg_Intrinsic;
+  def int_aarch64_neon_abs : AdvSIMD_1Arg_Intrinsic;
----------------
Forgot to remove this?


https://reviews.llvm.org/D41792