[PATCH] D70545: [ARM][MVE][Intrinsics] Add MVE VABD intrinsics.

Simon Tatham via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Nov 26 03:30:09 PST 2019


simon_tatham added a comment.

This mostly LGTM: only a handful of nits.



================
Comment at: clang/include/clang/Basic/arm_mve.td:45
 let params = T.Usual in {
+def vabdq: Intrinsic<Vector, (args Vector:$a, Vector:$b), (IRInt<"vabd", [Vector]> $a, $b)>;
+}
----------------
Can you wrap this line to 80 columns, please? I've been trying to fit the rest of the file in that width.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:1669
+                     list<dag> pattern=[]>
+  : MVE_int<iname, suffix, size, pattern> {
 
----------------
This new template parameter `iname` seems to be redundant, since I can't see anywhere you set it to anything other than `"vabd"`.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:2958
+class MVE_VABD_fp<string iname, string suffix, bit size>
+  : MVE_float<iname, suffix, (outs MQPR:$Qd), (ins MQPR:$Qn, MQPR:$Qm),
               "$Qd, $Qn, $Qm", vpred_r, ""> {
----------------
Similarly here: `iname` is an unnecessary extra template parameter.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70545/new/

https://reviews.llvm.org/D70545





More information about the cfe-commits mailing list