[PATCH] D148118: [AArch64][DAGCombiner]: combine <2xi64> mul add/sub.

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17817
+    return SDValue();
+
+  if (N->getOpcode() != ISD::ADD && N->getOpcode() != ISD::SUB)
----------------
Given the `MUL_PRED` node you care about is the result of operation legalisation it's perhaps worth having a `!isBeforeLegalizeOps()` or `isAfterLegalizeDAG()` check so the combine costs less compile time during the earlier code generation phases.  It'll also mean you can be sure the types are legal just in case we get here via some weird route.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17826-17836
+  if (N->getOperand(0)->getOpcode() == ISD::EXTRACT_SUBVECTOR) {
+    if(N->getOpcode() == ISD::SUB)
+      // That invalidate the pattern for mls
+      return SDValue();
+    ExtractOp = N->getOperand(0);
+    Op = N->getOperand(1);
+  } else if (N->getOperand(1)->getOpcode() == ISD::EXTRACT_SUBVECTOR) {
----------------
I think you'll cover more cases if you check `N->getOperand(1)` first.  For example, consider the case when you have `sub(extract(), extract(mul_pred()))`, with the current order (i.e. checking Op0 first) the combine will not fire?

I guess there's an argument it would be even better for the function to take the operands as parameters (or implement a lambda function) so you'd have something like:
```
if (res = performOpt(Opcode, Op0, Op1)
  return res;
if (Opcode == ISD::ADD)
  if (res = performOpt(Opcode, Op1, Op0)
    return res;
```
This way you'll cover the maximum number of combinations.

I'll leave the decision whether it's worth covering all cases to you but as a minimum I'd switch to check Op1 first as mention above.


 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148118