[PATCH 2/4] SelectionDAG: Expand SELECT_CC to SELECT + SETCC
Pete Cooper
peter_cooper at apple.com
Mon Jun 9 13:48:17 PDT 2014
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>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140609/bb89e914/attachment.html>
More information about the llvm-commits
mailing list