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

[Jolanta Jensen via Phabricator via llvm-commits]

jolanta.jensen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1278
 
+static std::optional<Instruction *> instCombineSVEAllActive(IntrinsicInst &II,
+                                                            Intrinsic::ID IID) {
----------------
paulwalker-arm wrote:
> 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).".
Added. 


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1295
+                                                            Intrinsic::ID IID) {
+  if (II.getIntrinsicID() != IID)
+    if (auto II_U = instCombineSVEAllActive(II, IID))
----------------
paulwalker-arm wrote:
> I guess this test makes the functions easier to use.  Perhaps move the test into `instCombineSVEAllActive` so the slight weirdness is hidden?
If I move the test to `instCombineAllActive()` I will not know if the returned intrinsic is the renamed one or the same _u intrinsic I got via the argument list here. If I already got an _u intrinsic I want to execute the remaining code in this function, not to return the intrinsic. But I removed this test as of now, `instCombineVectorAdd()` will never be called for an _u intrinsics. However I still have this check in `instCombineSVEVectorMul()`, which is called with both _u and non _u intrinsics. Happy for any suggestions how it could be rewritten. 


================
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;
----------------
paulwalker-arm wrote:
> 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.
> 
> 
Right, I now recall a comment in https://reviews.llvm.org/D144413 about the issue. Corrected by rebase on top of rGb7287a82d33b


================
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;
----------------
paulwalker-arm wrote:
> jolanta.jensen wrote:
> > paulwalker-arm wrote:
> > > 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.
> > > 
> > > 
> > Right, I now recall a comment in https://reviews.llvm.org/D144413 about the issue. Corrected by rebase on top of rGb7287a82d33b
> I've committed https://reviews.llvm.org/rGb7287a82d33b to fix the bug introduced by D150768.
Thank you!


================
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;
----------------
paulwalker-arm wrote:
> Same comment as with `instCombineSVEVectorAdd`.
Corrected by rebase on top of rGb7287a82d33b


================
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);
----------------
paulwalker-arm wrote:
> Based on my comment above, I think this code has to say here and you'll want to add a call to `instCombineSVEAllActive`.
Corrected by rebase on top of rGb7287a82d33b


================
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);
----------------
paulwalker-arm wrote:
> As above.
Fixed by rebase on top of rGb7287a82d33b.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152005