[PATCH] D150768: [SVE ACLE] Canonicalise SVE merging intrinsics -- part 1

[Jolanta Jensen via Phabricator via llvm-commits]

jolanta.jensen added inline comments.


================
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;
----------------
paulwalker-arm wrote:
> 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);
> ```
> 
Yes, there was no fallout from the move. It looks much better now.


================
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;
----------------
paulwalker-arm wrote:
> Same comment as above relating to maintain the function original structure.
Fixed.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1326
+static std::optional<Instruction *>
+instCombineSVEAllActive2VA(InstCombiner &IC, IntrinsicInst &II) {
+  auto *OpPredicate = II.getOperand(0);
----------------
paulwalker-arm wrote:
> 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.
Removed IR combines from this patch.


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-fmul_u-idempotency.ll:3
+
+target triple = "aarch64-unknown-linux-gnu"
+
----------------
paulwalker-arm wrote:
> 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.
It looks like this test and sve-intrinsic-fma-binops.ll cover all paths taken in instCombineSVEVectorMul.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150768