[PATCH] D111638: [AArch64][SVE] Combine predicated FMUL/FADD into FMA

[Peter Waller via Phabricator via llvm-commits]

peterwaller-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:718
+  if (!match(AddOp1, m_Intrinsic<Intrinsic::aarch64_sve_fmul>()))
+    return None;
+
----------------
I'd expect this all to look simpler, please have a go at simplifying. I think you can drop the matching logic above and instead add a condition that checks the AddOp1's predicate matches the Add's predicate.

One issue I have is that both the swap and m_SVEFAdd as it currently is serve the purpose of testing both operand orders. I think it's important for clarity to only do this once.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:724
+  llvm::FastMathFlags flags = II.getFastMathFlags();
+  flags &= FMulInst->getFastMathFlags();
+
----------------
It seems to me that a check is needed if the fast math flags contain 'contract', and if not, bailout.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:754
+    auto FmlaResult = instCombineSVEVectorFmla(IC, II);
+    if (FmlaResult.hasValue())
+      return FmlaResult;
----------------
Additionally to the diff-suggestion above, I think you could just call this FMLA. (I'd prefer to see FMLA capitalized in this context since it is an abbreviation so it matches, e.g. "SVE" in style.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:756
+      return FmlaResult;
+  }
+
----------------
I think I would prefer to see this in the big-switch-of-intrinsics or alternatively in a function called instCombineSVEVectorFAdd. It feels wrong for it to appear inside VectorBinOp since it doesn't apply to any BinOp other than FAdd, so the "hierarchy" is wrong in my view.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111638