[PATCH] D147236: [AArch64][Combine]: combine <2xi64> Mul-Add.

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17837
+
+  if(!MulValue.hasOneUse() || !N->getOperand(0).hasOneUse())
+    return SDValue();
----------------
Hi @hassnaa-arm, it looks like you're trying to check the use of both the add/sub operands, but you're potentially only checking one. For example, `MulValue` could have come from `N->getOperand(0)`. I think we may only need to check for multiple uses of the mul, because if it's used more than once we probably want to calculate the mul separately and reuse the result rather than recalculating it in several mla/mls instructions.

Also, I imagine that `MulValue` only ever has one use because the sequence will be

  %MulValue = mul <vscale x 2 x i64> ...
  %ExtractLowMul = <2 x i64> extract_subvector <vscale x 2 x i64> %MulValue, i64 0
  %Add = add <2 x i64> %ExtractLowMul, %AddOp

I think what you probably should be testing for here is one use of `%ExtractLowMul`, since that's the thing likely to get reused.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17850
+
+  SDValue ScaledConstValue = convertToScalableVector(DAG, VT, AddOp);
+  SDValue Add =
----------------
nit: Sorry, I only just spotted this. Could you rename this to `ScaledAddOp` instead?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17850
+
+  SDValue ScaledConstValue = convertToScalableVector(DAG, VT, AddOp);
+  SDValue Add =
----------------
david-arm wrote:
> nit: Sorry, I only just spotted this. Could you rename this to `ScaledAddOp` instead?
Again, sorry but I just realised that `convertToScalableVector` expects `AddOp` to have a fixed-length vector type. In theory, we could match the same pattern for just scalable vectors, i.e.

  %MulValue = mul <vscale x 2 x i64> ...
  %ExtractLowMul = <vscale x 2 x i64> extract_subvector <vscale x 2 x i64> %MulValue, i64 0
  %Add = add <vscale x 2 x i64> %ExtractLowMul, %AddOp

so it's worth checking for the type of the node (N) result at the start of `performMulAddSubCombine`, i.e. something like

  if (N->getOpcode() != ISD::ADD && N->getOpcode() != ISD::SUB)
    return SDValue();

  if (!N->getValueType(0).isFixedLengthVector())
    return SDValue();


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147236