[PATCH 2/4] SelectionDAG: Expand SELECT_CC to SELECT + SETCC
Tom Stellard
tom at stellard.net
Tue Jun 10 12:54:13 PDT 2014
r210539
On Mon, Jun 09, 2014 at 01:48:17PM -0700, Pete Cooper wrote:
> Hey Tom
>
> Thanks, using Expand here makes sense.
>
> So everything LGTM.
>
> Thanks,
> Pete
> On Jun 9, 2014, at 1:45 PM, Tom Stellard <tom at stellard.net> wrote:
>
> > On Tue, May 13, 2014 at 12:39:43PM -0700, Pete Cooper wrote:
> >>
> >> On May 13, 2014, at 12:36 PM, Tom Stellard <tom at stellard.net> wrote:
> >>
> >>> 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.
> >> Yeah, its a pain we can’t do a better job of detecting the infinite loop. Thats not something to block this patch though, just an observation on legalization itself.
> >>
> >> The target independent bit LGTM.
> >>
> >
> > Hi Pete,
> >
> > I've done more testing with this, and I think the assert should be testing:
> >
> > !TLI.isOperationExpand(ISD::SELECT, VT)
> >
> > We should be able to Expand SELECT_CC if SELECT is marked
> > as Promote. For example, on R600, we promote f32 SELECT to i32 SELECT.
> > I have attached an update patch.
> >
> > Thanks,
> > Tom
> >
> >> Thanks,
> >> Pete
> >>>
> >>> 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>
> >>>>
> >>> <0001-SelectionDAG-Expand-SELECT_CC-to-SELECT-SETCC.patch>
> >>
> > <0001-SelectionDAG-Expand-SELECT_CC-to-SELECT-SETCC.patch>
>
More information about the llvm-commits
mailing list