[PATCH 2/4] SelectionDAG: Expand SELECT_CC to SELECT + SETCC
Tom Stellard
tom at stellard.net
Tue May 13 09:00:38 PDT 2014
On Mon, May 12, 2014 at 03:53:22PM -0700, Pete Cooper wrote:
> Hi Tom
>
> Sorry for the late comments.
>
> I can’t speak for the targets themselves but the LegalizeDAG part looks good, with just one comment...
> On May 5, 2014, at 8:17 PM, Tom Stellard <thomas.stellard at amd.com> wrote:
>
> > This consolidates code from the Hexagon, R600, and XCore targets.
> >
> > No functionality change intended.
> > ---
> > lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | 15 +++++++++++++++
> > lib/Target/Hexagon/HexagonISelLowering.cpp | 20 ++------------------
> > lib/Target/Hexagon/HexagonISelLowering.h | 1 -
> > lib/Target/R600/SIISelLowering.cpp | 22 ++++------------------
> > lib/Target/R600/SIISelLowering.h | 1 -
> > lib/Target/XCore/XCoreISelLowering.cpp | 13 +------------
> > lib/Target/XCore/XCoreISelLowering.h | 1 -
> > 7 files changed, 22 insertions(+), 51 deletions(-)
> >
> > diff --git a/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp b/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
> > index dac372f..745ed4a 100644
> > --- a/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
> > +++ b/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
> > @@ -3899,8 +3899,23 @@ void SelectionDAGLegalize::ExpandNode(SDNode *Node) {
> > Tmp2 = Node->getOperand(1); // RHS
> > Tmp3 = Node->getOperand(2); // True
> > Tmp4 = Node->getOperand(3); // False
> > + EVT VT = Node->getValueType(0);
> > SDValue CC = Node->getOperand(4);
> >
> > + // First check if SELECT_CC nodes are legal for this type
> > + if (!TLI.isOperationLegalOrCustom(ISD::SELECT_CC, VT)) {
> > + EVT CmpVT = Tmp1.getValueType();
> > + assert(TLI.isOperationLegalOrCustom(ISD::SELECT, VT) &&
> > + TLI.isOperationLegalOrCustom(ISD::SETCC, CmpVT) &&
> It looks like ExpandNode could handle SETCC being illegal here. An illegal SETCC will be further expanded to a SELET_CC. So we may want to remove the SETCC assert if you’re happy that we won’t hit infinite legalization. I don’t think we will, but its hard to be 100% sure.
>
Hi Pete,
I think it will lead to an infinite loop if we drop the assert, because
if the SETCC is lowered to an illegal SELECT_CC, then we will be right
back here where we started.
I did notice a different issue with the patch. We can't check for
if (!TLI.isOperationLegalOrCustom(ISD::SELECT_CC, VT)) {
Because we may need to Expand an node that is marked Custom in case the
target does nothing to the node in its custom lowering function.
Instead we must use:
if (TLI.isCondCodeLegal(CCOp, Tmp1.getSimpleValueType()))
Also, cc'ing code owners in case they have any comments.
-Tom
> Thanks,
> Pete
> > + "ISD::SELECT and ISD::SETCC must both be legal in order to expand "
> > + "ISD::SELECT_CC");
> > + EVT CCVT = TLI.getSetCCResultType(*DAG.getContext(), CmpVT);
> > + SDValue Cond = DAG.getNode(ISD::SETCC, dl, CCVT, Tmp1, Tmp2, CC);
> > + Results.push_back(DAG.getSelect(dl, VT, Cond, Tmp3, Tmp4));
> > + break;
> > + }
> > +
> > + // SELECT_CC is legal, so the condition code must not be.
> > bool Legalized = false;
> > // Try to legalize by inverting the condition. This is for targets that
> > // might support an ordered version of a condition, but not the unordered
> > diff --git a/lib/Target/Hexagon/HexagonISelLowering.cpp b/lib/Target/Hexagon/HexagonISelLowering.cpp
> > index b8e5d24..aa4e9b5 100644
> > --- a/lib/Target/Hexagon/HexagonISelLowering.cpp
> > +++ b/lib/Target/Hexagon/HexagonISelLowering.cpp
> > @@ -944,21 +944,6 @@ HexagonTargetLowering::LowerVASTART(SDValue Op, SelectionDAG &DAG) const {
> > }
> >
> > SDValue
> > -HexagonTargetLowering::LowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const {
> > - SDValue LHS = Op.getOperand(0);
> > - SDValue RHS = Op.getOperand(1);
> > - SDValue CC = Op.getOperand(4);
> > - SDValue TrueVal = Op.getOperand(2);
> > - SDValue FalseVal = Op.getOperand(3);
> > - SDLoc dl(Op);
> > - SDNode* OpNode = Op.getNode();
> > - EVT SVT = OpNode->getValueType(0);
> > -
> > - SDValue Cond = DAG.getNode(ISD::SETCC, dl, MVT::i1, LHS, RHS, CC);
> > - return DAG.getNode(ISD::SELECT, dl, SVT, Cond, TrueVal, FalseVal);
> > -}
> > -
> > -SDValue
> > HexagonTargetLowering::LowerConstantPool(SDValue Op, SelectionDAG &DAG) const {
> > EVT ValTy = Op.getValueType();
> > SDLoc dl(Op);
> > @@ -1341,8 +1326,8 @@ HexagonTargetLowering::HexagonTargetLowering(HexagonTargetMachine
> > setOperationAction(ISD::BSWAP, MVT::i64, Expand);
> >
> > // Lower SELECT_CC to SETCC and SELECT.
> > - setOperationAction(ISD::SELECT_CC, MVT::i32, Custom);
> > - setOperationAction(ISD::SELECT_CC, MVT::i64, Custom);
> > + setOperationAction(ISD::SELECT_CC, MVT::i32, Expand);
> > + setOperationAction(ISD::SELECT_CC, MVT::i64, Expand);
> >
> > if (QRI->Subtarget.hasV5TOps()) {
> >
> > @@ -1577,7 +1562,6 @@ HexagonTargetLowering::LowerOperation(SDValue Op, SelectionDAG &DAG) const {
> > case ISD::BR_JT: return LowerBR_JT(Op, DAG);
> >
> > case ISD::DYNAMIC_STACKALLOC: return LowerDYNAMIC_STACKALLOC(Op, DAG);
> > - case ISD::SELECT_CC: return LowerSELECT_CC(Op, DAG);
> > case ISD::SELECT: return Op;
> > case ISD::INTRINSIC_WO_CHAIN: return LowerINTRINSIC_WO_CHAIN(Op, DAG);
> > case ISD::INLINEASM: return LowerINLINEASM(Op, DAG);
> > diff --git a/lib/Target/Hexagon/HexagonISelLowering.h b/lib/Target/Hexagon/HexagonISelLowering.h
> > index 4f27c27..0ddaf84 100644
> > --- a/lib/Target/Hexagon/HexagonISelLowering.h
> > +++ b/lib/Target/Hexagon/HexagonISelLowering.h
> > @@ -124,7 +124,6 @@ namespace llvm {
> > const SmallVectorImpl<SDValue> &OutVals,
> > SDValue Callee) const;
> >
> > - SDValue LowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const;
> > SDValue LowerFRAMEADDR(SDValue Op, SelectionDAG &DAG) const;
> > SDValue LowerATOMIC_FENCE(SDValue Op, SelectionDAG& DAG) const;
> > SDValue LowerRETURNADDR(SDValue Op, SelectionDAG &DAG) const;
> > diff --git a/lib/Target/R600/SIISelLowering.cpp b/lib/Target/R600/SIISelLowering.cpp
> > index cacff83..57ac274 100644
> > --- a/lib/Target/R600/SIISelLowering.cpp
> > +++ b/lib/Target/R600/SIISelLowering.cpp
> > @@ -103,10 +103,10 @@ SITargetLowering::SITargetLowering(TargetMachine &TM) :
> > setOperationAction(ISD::SELECT, MVT::f64, Promote);
> > AddPromotedToType(ISD::SELECT, MVT::f64, MVT::i64);
> >
> > - setOperationAction(ISD::SELECT_CC, MVT::f32, Custom);
> > - setOperationAction(ISD::SELECT_CC, MVT::i32, Custom);
> > -
> > - setOperationAction(ISD::SELECT_CC, MVT::Other, Expand);
> > + setOperationAction(ISD::SELECT_CC, MVT::f32, Expand);
> > + setOperationAction(ISD::SELECT_CC, MVT::i32, Expand);
> > + setOperationAction(ISD::SELECT_CC, MVT::i64, Expand);
> > + setOperationAction(ISD::SELECT_CC, MVT::f64, Expand);
> >
> > setOperationAction(ISD::SETCC, MVT::v2i1, Expand);
> > setOperationAction(ISD::SETCC, MVT::v4i1, Expand);
> > @@ -601,7 +601,6 @@ SDValue SITargetLowering::LowerOperation(SDValue Op, SelectionDAG &DAG) const {
> > }
> >
> > case ISD::SELECT: return LowerSELECT(Op, DAG);
> > - case ISD::SELECT_CC: return LowerSELECT_CC(Op, DAG);
> > case ISD::SIGN_EXTEND: return LowerSIGN_EXTEND(Op, DAG);
> > case ISD::STORE: return LowerSTORE(Op, DAG);
> > case ISD::ANY_EXTEND: // Fall-through
> > @@ -893,19 +892,6 @@ SDValue SITargetLowering::LowerSELECT(SDValue Op, SelectionDAG &DAG) const {
> > return DAG.getNode(ISD::BITCAST, DL, MVT::i64, Res);
> > }
> >
> > -SDValue SITargetLowering::LowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const {
> > - SDValue LHS = Op.getOperand(0);
> > - SDValue RHS = Op.getOperand(1);
> > - SDValue True = Op.getOperand(2);
> > - SDValue False = Op.getOperand(3);
> > - SDValue CC = Op.getOperand(4);
> > - EVT VT = Op.getValueType();
> > - SDLoc DL(Op);
> > -
> > - SDValue Cond = DAG.getNode(ISD::SETCC, DL, MVT::i1, LHS, RHS, CC);
> > - return DAG.getNode(ISD::SELECT, DL, VT, Cond, True, False);
> > -}
> > -
> > SDValue SITargetLowering::LowerSIGN_EXTEND(SDValue Op,
> > SelectionDAG &DAG) const {
> > EVT VT = Op.getValueType();
> > diff --git a/lib/Target/R600/SIISelLowering.h b/lib/Target/R600/SIISelLowering.h
> > index c6eaa81..3253f21 100644
> > --- a/lib/Target/R600/SIISelLowering.h
> > +++ b/lib/Target/R600/SIISelLowering.h
> > @@ -27,7 +27,6 @@ class SITargetLowering : public AMDGPUTargetLowering {
> > SelectionDAG &DAG) const;
> > SDValue LowerLOAD(SDValue Op, SelectionDAG &DAG) const;
> > SDValue LowerSELECT(SDValue Op, SelectionDAG &DAG) const;
> > - SDValue LowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const;
> > SDValue LowerSIGN_EXTEND(SDValue Op, SelectionDAG &DAG) const;
> > SDValue LowerSTORE(SDValue Op, SelectionDAG &DAG) const;
> > SDValue LowerZERO_EXTEND(SDValue Op, SelectionDAG &DAG) const;
> > diff --git a/lib/Target/XCore/XCoreISelLowering.cpp b/lib/Target/XCore/XCoreISelLowering.cpp
> > index 1c52c70..96eebb4 100644
> > --- a/lib/Target/XCore/XCoreISelLowering.cpp
> > +++ b/lib/Target/XCore/XCoreISelLowering.cpp
> > @@ -92,7 +92,7 @@ XCoreTargetLowering::XCoreTargetLowering(XCoreTargetMachine &XTM)
> >
> > // XCore does not have the NodeTypes below.
> > setOperationAction(ISD::BR_CC, MVT::i32, Expand);
> > - setOperationAction(ISD::SELECT_CC, MVT::i32, Custom);
> > + setOperationAction(ISD::SELECT_CC, MVT::i32, Expand);
> > setOperationAction(ISD::ADDC, MVT::i32, Expand);
> > setOperationAction(ISD::ADDE, MVT::i32, Expand);
> > setOperationAction(ISD::SUBC, MVT::i32, Expand);
> > @@ -217,7 +217,6 @@ LowerOperation(SDValue Op, SelectionDAG &DAG) const {
> > case ISD::BR_JT: return LowerBR_JT(Op, DAG);
> > case ISD::LOAD: return LowerLOAD(Op, DAG);
> > case ISD::STORE: return LowerSTORE(Op, DAG);
> > - case ISD::SELECT_CC: return LowerSELECT_CC(Op, DAG);
> > case ISD::VAARG: return LowerVAARG(Op, DAG);
> > case ISD::VASTART: return LowerVASTART(Op, DAG);
> > case ISD::SMUL_LOHI: return LowerSMUL_LOHI(Op, DAG);
> > @@ -259,16 +258,6 @@ void XCoreTargetLowering::ReplaceNodeResults(SDNode *N,
> > //===----------------------------------------------------------------------===//
> >
> > SDValue XCoreTargetLowering::
> > -LowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const
> > -{
> > - SDLoc dl(Op);
> > - SDValue Cond = DAG.getNode(ISD::SETCC, dl, MVT::i32, Op.getOperand(2),
> > - Op.getOperand(3), Op.getOperand(4));
> > - return DAG.getNode(ISD::SELECT, dl, MVT::i32, Cond, Op.getOperand(0),
> > - Op.getOperand(1));
> > -}
> > -
> > -SDValue XCoreTargetLowering::
> > getGlobalAddressWrapper(SDValue GA, const GlobalValue *GV,
> > SelectionDAG &DAG) const
> > {
> > diff --git a/lib/Target/XCore/XCoreISelLowering.h b/lib/Target/XCore/XCoreISelLowering.h
> > index 4e662fc..fc8b364 100644
> > --- a/lib/Target/XCore/XCoreISelLowering.h
> > +++ b/lib/Target/XCore/XCoreISelLowering.h
> > @@ -157,7 +157,6 @@ namespace llvm {
> > SDValue LowerBlockAddress(SDValue Op, SelectionDAG &DAG) const;
> > SDValue LowerConstantPool(SDValue Op, SelectionDAG &DAG) const;
> > SDValue LowerBR_JT(SDValue Op, SelectionDAG &DAG) const;
> > - SDValue LowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const;
> > SDValue LowerVAARG(SDValue Op, SelectionDAG &DAG) const;
> > SDValue LowerVASTART(SDValue Op, SelectionDAG &DAG) const;
> > SDValue LowerUMUL_LOHI(SDValue Op, SelectionDAG &DAG) const;
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-SelectionDAG-Expand-SELECT_CC-to-SELECT-SETCC.patch
Type: text/x-diff
Size: 10577 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140513/71cd2dff/attachment.patch>
More information about the llvm-commits
mailing list