[PATCH] D135615: [AArch64][ARM] Alter v8.1a neon intrinsics to be target-based, not preprocessor based

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:5854
+  NEONMAP1(vqrdmlshq_s16, arm_neon_vqrdmlsh, Add1ArgType),
+  NEONMAP1(vqrdmlshq_s32, arm_neon_vqrdmlsh, Add1ArgType),
   NEONMAP1(vqrdmulh_v, arm_neon_vqrdmulh, Add1ArgType),
----------------
simon_tatham wrote:
> This patch mostly looks comprehensible to me, but I don't understand what this change has to do with it. Everything else seems to be controlling how intrinsics are enabled/disabled, but the changes in this file seem to be renaming some intrinsics, and there's nothing about that in the commit message.
> 
> Is this intentionally part of the same patch?
It is to do with the tablegen emitter and how the `__builtin_arm_vqrdmlshq_s32` are defined, vs the old `__builtin_arm_vqrdmlshq_v` name. It comes from https://reviews.llvm.org/D132034. 

The short version is that because some intrinsics need different target features for different types, they cannot share the same _v builtin names. For example the _f16 or _bf16 builtins need different features to _f32, using a `TARGET_BUILTIN` in the generated files, as opposed to `BUILTIN`. This means that anything that has a TargetGuard no longer uses the _v common name, and in cases like this we end up with different names. In this case they could probably share the name again as all intrinsics require the same features, but as there are only 2 names I chose to keep it simple and just expand the NEONMAP for the new names.


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

https://reviews.llvm.org/D135615