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

[Allen zhong via Phabricator via llvm-commits]

Allen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14627
+  ConstantSDNode *CC = cast<ConstantSDNode>(CCVal);
+  assert((CC->isOne() || CC->isZero()) && "Unexpected constant vluae");
+
----------------
dmgreen wrote:
> I don't think this will only be EQ or NE. It can be any of the predicates.
> 
> The condition code is usually obtained via `static_cast<AArch64CC::CondCode>(OpCC->getZExtValue())` or something like it.
done, delete the assert after use API static_cast<AArch64CC::CondCode>(OpCC->getZExtValue())


================
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:
----------------
dmgreen wrote:
> 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?
valid for all conditions, Both of them  are add in test file aarch64-isel-csinc.ll


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14623
+      !CSneg.hasOneUse() || !Copy.hasOneUse() ||
+      Copy.getOpcode() != ISD::CopyFromReg)
+    return SDValue();
----------------
dmgreen wrote:
> Why does this only trigger from a CopyFromReg?
> Should we be checking for commutative Add operands?
> Why does this only trigger from a CopyFromReg?
 this may be can expand later, but now I don't have such case.

>Should we be checking for commutative Add operands?
As it is already restricted scalar Integer type, I think it would be commutative add?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14626
+
+  ISD::CondCode CC = ISD::SETNE;
+  // The csneg/csel should include a const one operand.
----------------
dmgreen wrote:
> 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.
Yes, added checking conditon. and  thanks for you detail advice  of using AArch64CC::getInvertedCondCode .


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

https://reviews.llvm.org/D116915