[PATCH] D111638: [AArch64][SVE] Combine predicated FMUL/FADD into FMA
Peter Waller via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 13 08:59:20 PDT 2021
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
More information about the llvm-commits
mailing list