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

[Matt Devereau via Phabricator via llvm-commits]

MattDevereau marked 5 inline comments as done.
MattDevereau added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:718
+  if (!match(AddOp1, m_Intrinsic<Intrinsic::aarch64_sve_fmul>()))
+    return None;
+
----------------
peterwaller-arm wrote:
> peterwaller-arm wrote:
> > 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.
> If you want to use the match syntax above I think you can also bind the fmul with `m_And(m_Value(FMul), m_SVEFMul(m_Deferred(p), m_Value(a), m_Value(b))` and then you could grab `FMul` and `c` simultaneously with the existing logic.
I've opted to check AddOp1's predicate matches the Add's predicate


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:724
+  llvm::FastMathFlags flags = II.getFastMathFlags();
+  flags &= FMulInst->getFastMathFlags();
+
----------------
peterwaller-arm wrote:
> peterwaller-arm wrote:
> > It seems to me that a check is needed if the fast math flags contain 'contract', and if not, bailout.
> Please also check that the flags are equal instead of taking their intersection. The fadd might plausibly have a flag which allows more aggressive optimization and contracting it in this way may prevent those optimizations from taking place.
I'm not sure what you mean here. Since flags is its own fresh variable isnt the intersection the same as comparing if the flags are equal? The original flags on the fadd intrinsic will be preserved


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111638