[PATCH] D100772: [ARM] Neon Polynomial vadd Intrinsic fix

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

I'd merge https://reviews.llvm.org/D100499 into this given that it's only one line.



================
Comment at: clang/include/clang/Basic/arm_neon.td:712
+////////////////////////////////////////////////////////////////////////////////
+// Crypto
+// TODO: poly128_t not implemented on aarch32
----------------
This isn't a crypto intrinsic, it just happens to be right after a block like:
```
let ArchGuard = "__ARM_ARCH >= 8 && defined(__ARM_FEATURE_CRYPTO)" in {
<...>
def SM4EKEY : SInst<"vsm4ekey", "...", "QUi">;
}
```

You could instead say:
```
// Non poly128_t vaddp for Arm and AArch64
```
(keep the todo)


================
Comment at: clang/include/clang/Basic/arm_neon.td:1168
 
-def VADDP   : WInst<"vadd", "...", "PcPsPlQPcQPsQPlQPk">;
+def VADDP   : WInst<"vadd", "...", "QPk">;
 
----------------
Add a comment here, something like `poly128_t vadd for AArch64 only, see VADDP_Q for the rest`.

Also the def names seem backwards, shouldn't this one be `VADDP_Q` and the one for Arm and AArch64 be `VADDP`?

Then your comment here is "see VADDP for the rest" which seems correct.


================
Comment at: clang/test/CodeGen/arm-poly-add.c:4
+// RUN:   -target-feature +neon \
+// RUN:   -target-feature +bf16 -mfloat-abi hard \
+// RUN: -disable-O0-optnone -emit-llvm -o - %s | opt -S -mem2reg \
----------------
You can remove the `+bf16` feature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100772