[PATCH 2/4] SelectionDAG: Expand SELECT_CC to SELECT + SETCC
Tom Stellard
tom at stellard.net
Tue May 13 12:36:46 PDT 2014
On Tue, May 13, 2014 at 10:04:28AM -0700, Pete Cooper wrote:
>
> On May 13, 2014, at 9:00 AM, Tom Stellard <tom at stellard.net> wrote:
>
> > 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.
> It all depends on the type of CCVT. You may have something like
>
> i32 selectcc = i8 cond, i32 true, i32 false
>
> If you have i8 selectcc legal then you could have safely created the i8 setcc even though its not legal or custom.
>
That's right, so if we remove the assert, there is a chance that someone
could hit an infinite loop. I think that's better than incorrectly
asserting when we could legalize something, and it will be very
difficult to detect when the infinite loop may occur.
Here is an updated patch with the assert removed.
-Tom
> Thanks,
> Pete
> >
> > 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
> > <0001-SelectionDAG-Expand-SELECT_CC-to-SELECT-SETCC.patch>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-SelectionDAG-Expand-SELECT_CC-to-SELECT-SETCC.patch
Type: text/x-diff
Size: 10473 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140513/64a2a242/attachment.patch>
More information about the llvm-commits
mailing list