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

[Matt Devereau via Phabricator via llvm-commits]

MattDevereau added inline comments.


================
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);
+  };
----------------
paulwalker-arm wrote:
> 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.
That sounds right but it seems like something that would probably be caught by a test somewhere. I've had a look at FMLA [[ https://developer.arm.com/documentation/ddi0602/2021-09/SVE-Instructions/FMLA--vectors---Floating-point-fused-multiply-add-vectors--predicated---writing-addend--Zda---Zda---Zn---Zm--?lang=en | here ]] and it says "Inactive elements in the destination vector register remain unmodified.". FADD on the otherhand places the value of the first data operand in the destination vector, so FMLA and FADD seem to differ here. I'm probably wrong but from what I can see, the addition part of the FMLA can assume `fadd(p, a, b) == fadd(p, b, a)` but a normal FADD instruction cannot? Either way it seems like a more extensive set of rules should be considered for this


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:724
+    return None;
+  bool AllowReassoc = FAddFlags.allowReassoc() && FMulFlags.allowReassoc();
+  bool AllowContract = FAddFlags.allowContract() && FMulFlags.allowContract();
----------------
paulwalker-arm wrote:
> Given the above I don't think we need `AllowReassoc` given we shouldn't be changing the order of operations. 
This seems linked to the other comment, which i think needs a bit more consideration




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111638