[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:17826-17833
+  if (N->getOperand(0)->getOpcode() == ISD::EXTRACT_SUBVECTOR) {
+    ExtractOp = N->getOperand(0);
+    Op = N->getOperand(1);
+  } else if (N->getOperand(1)->getOpcode() == ISD::EXTRACT_SUBVECTOR) {
+    ExtractOp = N->getOperand(1);
+    Op = N->getOperand(0);
+  } else
----------------
paulwalker-arm wrote:
> Hi @hassnaa-arm, I think this patch should be reverted and fixed before re-landing because the optimisation doesn't look sound.  The highlighted code block allows the original operands for `N` to be swapped, which whilst correct for commutative operations like `ISD::ADD` it is incorrect for `ISD::SUB` which this patch also supports.  You can see the bogus result by looking at the tests.  For example, `test_mul_sub_1x64` shows the IR for `(b * c) - a` which is not an `mls` operation, that would be `a - (b * c)`.
Ah good spot @paulwalker-arm! This is partly my fault too since I asked @hassnaa-arm to include the `ISD::SUB` case, but forgot about the fact we can't allow operands to be swapped for `ISD::SUB`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147236