[PATCH] D112204: [AArch64] Fold neg(csel(neg(a), b)) to csel(a, neg(b))

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Looks like a useful optimisation @dmgreen! I left a few minor comments ...



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14368
+static SDValue performNegCSelCombine(SDNode *N, SelectionDAG &DAG) {
+  if (N->getOpcode() != ISD::SUB || !isNullConstant(N->getOperand(0)))
+    return SDValue();
----------------
Is it worth having a simple helper function like `isNegate` for all these cases. Perhaps one already exists?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14378
+
+  if ((N0.getOpcode() != ISD::SUB || !isNullConstant(N0.getOperand(0))) &&
+      (N1.getOpcode() != ISD::SUB || !isNullConstant(N1.getOperand(0))))
----------------
Does this work when both inputs are negated and do we have a test for that?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14384
+  SDLoc DL(CSel);
+  SDValue N0N = DAG.getNode(N->getOpcode(), DL, VT, N->getOperand(0), N0);
+  SDValue N1N = DAG.getNode(N->getOpcode(), DL, VT, N->getOperand(0), N1);
----------------
I understand you're reusing the opcode and operands here, but it took me a while to realise that! Is it worth just writing out what we're doing here fully for clarity, i.e. something a bit like

  SDValue Zero = N->getOperand(0);
  SDValue N0N = DAG.getNode(ISD::SUB, DL, VT, Zero, N0);
  SDValue N1N = DAG.getNode(ISD::SUB, DL, VT, Zero, N1);


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

https://reviews.llvm.org/D112204