[PATCH] D140200: [AArch64][InstCombine] Fuse ADD+MUL and SUB+MUL AArch64 instrinsics

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1051
+instCombineSVEVectorFuseMulAddSub(InstCombiner &IC, IntrinsicInst &II,
+                                  bool IsAddend) {
   Value *P = II.getOperand(0);
----------------
nit: Perhaps `MergeIntoAddendOp` is a better name?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1053
   Value *P = II.getOperand(0);
-  Value *A = II.getOperand(1);
-  auto FMul = II.getOperand(2);
-  Value *B, *C;
-  if (!match(FMul, m_Intrinsic<Intrinsic::aarch64_sve_fmul>(
-                       m_Specific(P), m_Value(B), m_Value(C))))
-    return std::nullopt;
+  Value *A, *B, *C, *Mul;
+  if (IsAddend) {
----------------
Could you rename these variables such that:

  A -> MulOp0
  B -> MulOp1
  C -> AddendOp

That makes the code below (where you swap the order of these in `{P, C, A, B}` for example,  a bit easier to follow)


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1224
+    return FMSB;
   return instCombineSVEVectorBinOp(IC, II);
 }
----------------
nit: Can you add a comment saying that there is no integer version of nmsb for the equivalent integer case of the above?

(is there a negative test available for that case?)


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-muladdsub.ll:397
+
+define dso_local <vscale x 16 x i8> @combine_mls_i8(<vscale x 16 x i1> %p, <vscale x 16 x i8> %a, <vscale x 16 x i8> %b, <vscale x 16 x i8> %c) local_unnamed_addr #0 {
+; CHECK-LABEL: @combine_mls_i8(
----------------
I don't think we need these tests for all the element types, probably doing this for 1 FP type and 1 Integer type is sufficient.


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-muladdsub.ll:516
+
+define dso_local <vscale x 8 x half> @neg_combine_fnmsb_two_fmul_uses(<vscale x 16 x i1> %p, <vscale x 8 x half> %a, <vscale x 8 x half> %b, <vscale x 8 x half> %c) local_unnamed_addr #0 {
+; CHECK-LABEL: @neg_combine_fnmsb_two_fmul_uses(
----------------
I don't necessarily think you need to repeat these negative tests for all the fmla/fmad/fmls/.. combinations (same for the tests above (predicate not matching, not the right fast-math flags, etc). Maybe you can have all positive tests firsts, and then all the negative tests for just the `fmla`, since they should directly translate to the other mul-add intrinsics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140200