[PATCH] D60485: [AArch64] Add support for MTE intrinsics

[Tim Northover via Phabricator via cfe-commits]

t.p.northover added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9566
+def err_memtag_arg_null_or_pointer : Error<
+  "%0 argument must be a null or a pointer (%1 invalid)">;
+def err_memtag_any2arg_pointer : Error<
----------------
javed.absar wrote:
> t.p.northover wrote:
> > I think these diagnostics could do with a bit more context for consistency. They seem to take "MTE builtin" for granted, whereas most Clang messages mention what they're talking about.
> > 
> > I'm not saying "MTE builtin" is the best we can come up with, BTW, just that something more would be nice.
> I thought of doing that too , so can prefix these warnings with 'mte builtin' if that's what you meant. But the intrinsic called kind of names same thing (__arm_mte_..).
"Builtin" or even "function" (fixed up to be grammatical) would suffice if you don't like the repetition.


================
Comment at: lib/Headers/arm_acle.h:610-615
+#define __arm_mte_create_random_tag(__ptr, __mask)  __builtin_arm_irg(__ptr, __mask)
+#define __arm_mte_increment_tag(__ptr, __tag_offset)  __builtin_arm_addg(__ptr, __tag_offset)
+#define __arm_mte_exclude_tag(__ptr, __excluded)  __builtin_arm_gmi(__ptr, __excluded)
+#define __arm_mte_get_tag(__ptr) __builtin_arm_ldg(__ptr)
+#define __arm_mte_set_tag(__ptr) __builtin_arm_stg(__ptr)
+#define __arm_mte_ptrdiff(__ptra, __ptrb) __builtin_arm_subp(__ptra, __ptrb)
----------------
javed.absar wrote:
> t.p.northover wrote:
> > Why are the builtin names so different from the ones exposed. GCC compatibility? LLVM?
> The builtin name (e.g. _mte_irg) is reflecting the instruction that implements the otherwise meaningful ACLE names  (mte_create_tag). Its just that the instruction names are sometimes cryptic (e.g. stg, ldg). I could change the names to __builtin_arm_create_tag etc and push the meaningful name -> insn level name to intrinsic level or further down but that would mean lots of name changes to current patch and tests. 
OK, sounds reasonable to leave it as is then.


================
Comment at: lib/Sema/SemaChecking.cpp:6112
+/// SemaBuiltinARMMemoryTaggingCall - Handle calls of memory tagging extensions
+bool Sema::SemaBuiltinARMMemoryTaggingCall( unsigned BuiltinID, CallExpr *TheCall) {
+  bool IsMTEBuiltin = BuiltinID == AArch64::BI__builtin_arm_irg ||
----------------
Stray space here between '(' and 'unsigned', BTW.


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

https://reviews.llvm.org/D60485