[PATCH] D78401: [SveEmitter] IsInsertOp1SVALL and builtins for svqdec[bhwd] and svqinc[bhwd]

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added inline comments.


================
Comment at: clang/include/clang/Basic/arm_sve.td:530
+class sat_type<string u, string t> { string U = u; string T = t; }
+def SIGNED_BYTE         : sat_type<"",  "c">;
+def SIGNED_HALF         : sat_type<"",  "s">;
----------------
SjoerdMeijer wrote:
> nit: just wondering if all these defs should be all capitals.
They don't and I can see how this can be confused with intrinsics. I've changed it!


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7684
 
+    if (TypeFlags.isInsertOp1SVALL())
+      Ops.insert(&Ops[1], Builder.getInt32(31));
----------------
SjoerdMeijer wrote:
> would this be the most appropriate place to add the useful sentence from the description: 
> 
> "Some ACLE builtins leave out the argument to specify the predicate
> pattern, which is expected to be expanded to an SV_ALL pattern."
> 
> because that's what 31 is, right?
Yes, good suggestion! I've added the comment.


================
Comment at: clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_qdecb.c:7
+#ifdef SVE_OVERLOADED_FORMS
+// A simple used,unused... macro, long enough to represent any SVE builtin.
+#define SVE_ACLE_FUNC(A1,A2_UNUSED,A3,A4_UNUSED) A1##A3
----------------
SjoerdMeijer wrote:
> nit: used,unused -> "used/unused", or "used, unused"
I'd rather not fix this for this patch, because it would need changing for all other test files as well.


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

https://reviews.llvm.org/D78401