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

David Sherwood via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Apr 11 08:29:56 PDT 2023

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`.

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list