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

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

This is looking much better now @hassnaa-arm - thanks for making the changes! I just have a few more minor comments, but then it looks good to go.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17807
 
+// This works on the pattern of: add v1 , ( mul v2, v3 ),
+// It will transform the add to scalable version, so that
----------------
nit: This is just a suggestion, but perhaps this version of the comment is more explicit:

  // This works on the patterns of:
  //   add v1, (mul v2, v3)
  //   sub v1, (mul v2, v3)
  // for vectors of type <1 x i64> and <2 x i64> when SVE is available. It will
  // transform the add/sub to a scalable version, so that we can make use of
  // SVE's MLA/MLS that will be generated for that pattern.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17819
+
+  SDValue MulValue, AddConstValue, ExtractIndexValue;
+  if (N->getOperand(0)->getOpcode() == ISD::EXTRACT_SUBVECTOR) {
----------------
I still think it's a bit misleading to have 'Const' in the name because it's not necessarily a constant. For example, in your test `@test_mul_add_2x64` below the other operand is a function argument.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17841
+  // If the ConstValue is NOT 0, then that is NOT the expected pattern:
+  ConstantSDNode *CV = dyn_cast<ConstantSDNode>(ExtractIndexValue);
+  if(CV && CV->getSExtValue() != 0)
----------------
I think the definition of EXTRACT_SUBVECTOR says this must be a constant so you can just write this instead:

  if (!cast<ConstantSDNode>(ExtractIndexValue)->isZero())
    return SDValue();

Note you can just the `ConstantSDNode::isZero` function here instead as it's a bit simpler.


================
Comment at: llvm/test/CodeGen/AArch64/aarch64-combine-add-sub-mul.ll:56
+; CHECK-NEXT:    // kill: def $d1 killed $d1 def $z1
+; CHECK-NEXT:    mls z0.d, p0/m, z1.d, z2.d
+; CHECK-NEXT:    // kill: def $d0 killed $d0 killed $z0
----------------
Nice!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147236