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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

Sorry @MattDevereau for the too late review but this is the first time I've had chance to look at the patch and I think there's an issue that needs fixing.



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:702-713
+  auto m_SVEFAdd = [](auto p, auto w, auto x) {
+    return m_CombineOr(m_Intrinsic<Intrinsic::aarch64_sve_fadd>(p, w, x),
+                       m_Intrinsic<Intrinsic::aarch64_sve_fadd>(p, x, w));
+  };
+  auto m_SVEFMul = [](auto p, auto y, auto z) {
+    return m_Intrinsic<Intrinsic::aarch64_sve_fmul>(p, y, z);
+  };
----------------
Is this correct?  It looks like you're allowing the `fmul` to be either operand of the `fadd`, effectively saying `fadd(p, a, b) == fadd(p, b, a)`.  Although true for the active lanes, the inactive lanes take the value of the first data operand and so these two forms are not identical.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:724
+    return None;
+  bool AllowReassoc = FAddFlags.allowReassoc() && FMulFlags.allowReassoc();
+  bool AllowContract = FAddFlags.allowContract() && FMulFlags.allowContract();
----------------
Given the above I don't think we need `AllowReassoc` given we shouldn't be changing the order of operations. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111638