[PATCH] D150768: [SVE ACLE] Canonicalise SVE merging intrinsics

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

In D150768#4363685 <https://reviews.llvm.org/D150768#4363685>, @mgabka wrote:

> This patch for now covers only fadd/fadd_u, fsub/fsub_u and fmul/fmul_u, what about other intrinsics which are having both merging and _u variants I thought we want to replace all merging forms with _u forms when the predicate is all true  ? @paulwalker-arm what is your opinion here?

This patch is concerned with extending existing combines rather than adding new ones.  I do agree the summary should be better worded to reflect this.



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1289-1296
+  if (auto Replacement = instCombineSVEVectorBinOp(IC, II))
+    return Replacement;
+  if (auto FMLA_U =
+          instCombineSVEVectorFuseMulAddSub<Intrinsic::aarch64_sve_fmul_u,
+                                            Intrinsic::aarch64_sve_fmla_u>(
+              IC, II, true))
+    return FMLA_U;
----------------
Is it possible to maintain the original structure of this function by just adding the new `instCombineSVEVectorFuseMulAddSub` entry for aarch64_sve_fmul_u? e.g.

```
  if (auto FMLA_U =
          instCombineSVEVectorFuseMulAddSub<Intrinsic::aarch64_sve_fmul_u,
                                            Intrinsic::aarch64_sve_fmla_u>(
              IC, II, true))
    return FMLA_U;
  return instCombineSVEVectorBinOp(IC, II);
```



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1315-1322
+  if (auto Replacement = instCombineSVEVectorBinOp(IC, II))
+    return Replacement;
+  if (auto FMLS_U =
+          instCombineSVEVectorFuseMulAddSub<Intrinsic::aarch64_sve_fmul_u,
+                                            Intrinsic::aarch64_sve_fmls_u>(
+              IC, II, true))
+    return FMLS_U;
----------------
Same comment as above relating to maintain the function original structure.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1326
+static std::optional<Instruction *>
+instCombineSVEAllActive2VA(InstCombiner &IC, IntrinsicInst &II) {
+  auto *OpPredicate = II.getOperand(0);
----------------
I'd rather the `instCombineSVEAllActive2VA` and `instCombineSVEAllActive3VA` changes be rolled into a separate patch.  That way we have this patch focusing on the existing combines that are being ported to include the newer intrinsics and then a following patch to add new combines for everything else.


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-fma-binops.ll:199
+
+define <vscale x 2 x double> @no_replace_on_non_ptrue_all_u(<vscale x 2 x double> %a, <vscale x 2 x double> %b) #0 {
+; CHECK-LABEL: @no_replace_on_non_ptrue_all_u
----------------
Do you mind moving this negative test so it appears after the three positive ones?


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-fmul_u-idempotency.ll:3
+
+target triple = "aarch64-unknown-linux-gnu"
+
----------------
mgabka wrote:
> the test in this file do not match the description of the commit message, I think that is an optimization we want to cover separately, @paulwalker-arm wjhat is your opinion?
@mgabka: This patch is about extending existing combines to also cover their equivalent `_u` intrinsics.  The majority relate to replacing intrinsic calls with IR instructions but `instCombineSVEVectorMul` contains more combines.  I think it would be messier to try to artificially avoid these combines so am happy for this patch to include them, assuming they're genuinely applicable? with this patch adding the relevant tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150768