[PATCH] D116915: [DAGCombiner][AArch64] Enhance to support for scalar CSINC

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14610
+/// Perform the scalar expression (a ? b-c : b+1) combine in the form of:
+///   b + CSEL (-c, 1, a != 0) / b + CSNEG (1, c, a==0)
+/// into:
----------------
This might be easier to read as:
b + CSEL (c, 1, cc)  =>  CSINC(b+c, b, cc)
b + CSNEG (1, c, cc)  =>  CSINC(b-c, b, !cc)

Does the a!=0/a==0 matter? Or is it valid for all conditions?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14623
+      !CSneg.hasOneUse() || !Copy.hasOneUse() ||
+      Copy.getOpcode() != ISD::CopyFromReg)
+    return SDValue();
----------------
Why does this only trigger from a CopyFromReg?
Should we be checking for commutative Add operands?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14626
+
+  ISD::CondCode CC = ISD::SETNE;
+  // The csneg/csel should include a const one operand.
----------------
Should we be checking the input CC (operand 2) is NE?

It should probably be using AArch64CC::CondCode without needing to go back to ISD::CondCode, possibly using AArch64CC::getInvertedCondCode to invert the condition.


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

https://reviews.llvm.org/D116915