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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
