[PATCH] D116915: [DAGCombiner][AArch64] Enhance to support for scalar CSINC
Allen zhong via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Jan 22 02:43:49 PST 2022
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
More information about the llvm-commits
mailing list