[PATCH] D152005: [SVE ACLE] Implement IR combines to convert intrinsics used for _m C/C++ builtins

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1278
 
+static std::optional<Instruction *> instCombineSVEAllActive(IntrinsicInst &II,
+                                                            Intrinsic::ID IID) {
----------------
Please add a function description comment along the lines of "Canonicalise operations that take an all active predicate (e.g. sve.add -> sve.add_u).".


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1295
+                                                            Intrinsic::ID IID) {
+  if (II.getIntrinsicID() != IID)
+    if (auto II_U = instCombineSVEAllActive(II, IID))
----------------
I guess this test makes the functions easier to use.  Perhaps move the test into `instCombineSVEAllActive` so the slight weirdness is hidden?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1321-1325
+  if (auto MLA_U =
+          instCombineSVEVectorFuseMulAddSub<Intrinsic::aarch64_sve_mul_u,
+                                            Intrinsic::aarch64_sve_mla_u>(
+              IC, II, true))
+    return MLA_U;
----------------
I'm pretty sure it's not safe to move this code here because `instCombineSVEVectorAdd` is called for both `sve.add` and `sve.add_u`. If you consider:
```
sve.add(a, sve.mul.u(b, c))
```
here the inactive lanes of the result are defined to come from `a`.  However this code will combine the IR into:
```
sve.mla.u(a, b, c)
```
where the result for inactive lanes will be undefined. This is why the combine originally lived outside of `instCombineSVEVectorAdd` and must remain outside after this work.

This also means https://reviews.llvm.org/D150768 has introduced a bug that I missed during review.




================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1354-1358
+  if (auto MLS_U =
+          instCombineSVEVectorFuseMulAddSub<Intrinsic::aarch64_sve_mul_u,
+                                            Intrinsic::aarch64_sve_mls_u>(
+              IC, II, true))
+    return MLS_U;
----------------
Same comment as with `instCombineSVEVectorAdd`.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1708-1710
-    return instCombineSVEVectorFuseMulAddSub<Intrinsic::aarch64_sve_mul_u,
-                                             Intrinsic::aarch64_sve_mla_u>(
-        IC, II, true);
----------------
Based on my comment above, I think this code has to say here and you'll want to add a call to `instCombineSVEAllActive`.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1716-1718
-    return instCombineSVEVectorFuseMulAddSub<Intrinsic::aarch64_sve_mul_u,
-                                             Intrinsic::aarch64_sve_mls_u>(
-        IC, II, true);
----------------
As above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152005