[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:17820
+static SDValue performAddMulCombine(SDNode *N, SelectionDAG &DAG) {
+  if (N->getOpcode() != ISD::ADD)
+    return SDValue();
----------------
I think we can easily extend this to include `ISD::SUB` too while we're fixing the add case, right? We should also match to the `mls` instruction.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17823
+
+  SDValue MulValue, ConstValue;
+  if (N->getOperand(0)->getOpcode() == ISD::EXTRACT_SUBVECTOR) {
----------------
I `ConstValue` is a bit misleading here - isn't this the other operand for the add? Perhaps you can rename this to `AddValue`?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17824
+  SDValue MulValue, ConstValue;
+  if (N->getOperand(0)->getOpcode() == ISD::EXTRACT_SUBVECTOR) {
+    MulValue = N->getOperand(0).getOperand(0);
----------------
I think you also need to check what the extract subvector index is too - I think the index should be 0.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17832
+    return SDValue();
+
+  EVT ContainerVT = getContainerForFixedLengthVector(DAG, N->getValueType(0));
----------------
I think you need to also check the opcode of `MulValue` here otherwise we'll start matching any arbitrary pattern:

  add <2 x i64> (any_op <2 x i64> ...), %op2


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17833
+
+  EVT ContainerVT = getContainerForFixedLengthVector(DAG, N->getValueType(0));
+  SDValue ScaledConstValue =
----------------
I think we should really be checking the input VT used for EXTRACT_SUBVECTOR here too and only apply the optimisation if the input is a scalable type. Once you know the input VT you also don't need to recalculate it with `getContainerForFixedLengthVector` because the container VT should be the same, i.e.

  add (extract_subvector (<vscale x 2 x i64> %in), i64 0), %op2

where we know from the type of `%in` that `ContainerVT=<vscale x 2 x i64>`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147236