PATCH: SelectionDAG: Remove setOperationAction(ISD::SELECT_CC, MVT::Other, Expand) hack.

Owen Anderson resistor at mac.com
Mon May 26 21:02:43 PDT 2014 

I’m fine with this.

—Owen

On May 25, 2014, at 9:10 AM, Tom Stellard <tom at stellard.net> wrote:

> Ping.
> 
> On Thu, May 15, 2014 at 06:25:49AM -0700, Tom Stellard wrote:
>> Ping.
>> 
>> On Fri, May 09, 2014 at 07:16:12AM -0700, Tom Stellard wrote:
>>> cc Code Owners
>>> 
>>> On Mon, May 05, 2014 at 11:29:30AM -0700, Tom Stellard wrote:
>>>> Hi,
>>>> 
>>>> I've updated this patches series to fix a few more bugs that I've found, and
>>>> I've also updated the broken MIPS test case.
>>>> 
>>>> The first two patches which add support for expanding SELECT_CC nodes I sent
>>>> to the list in separate emails, because they are isolated from the MVT::Other
>>>> fix.
>>>> 
>>>> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140505/215830.html
>>>> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140505/215829.html
>>>> 
>>>> The two remaining patches which remove the ISD::SELECT_CC + MVT::Other hack are
>>>> attached to this email.
>>>> 
>>>> Thanks,
>>>> Tom
>>>> 
>>>> On Fri, May 02, 2014 at 02:35:53PM -0700, Tom Stellard wrote:
>>>>> Hi,
>>>>> 
>>>>> I've always had a lot of trouble with ISD::SELECT_CC nodes in the
>>>>> SelectionDAG, and I think part of the problem is that there is a special
>>>>> case for ISD::SELECT_CC where targets can use the MVT::Other type to
>>>>> specify one action for all types.  The attached patches remove this
>>>>> special case and will hopefully help to simplify the legalization and
>>>>> optimization of ISD::SELECT_CC.
>>>>> 
>>> 
>>> 
>>>> From eab653641dd21e11ee85ec9f5b45e1b3907e07fa Mon Sep 17 00:00:00 2001
>>>> From: Tom Stellard <thomas.stellard at amd.com>
>>>> Date: Mon, 5 May 2014 20:57:52 -0400
>>>> Subject: [PATCH 3/4] SelectionDAG: Enable (and (setcc x), (setcc y)) -> (setcc
>>>> (and x, y)) for vectors
>>>> 
>>>> This prevents a future commit from regressing:
>>>> 
>>>> test/CodeGen/R600/setcc-equivalent.ll
>>>> ---
>>>> lib/CodeGen/SelectionDAG/DAGCombiner.cpp | 8 ++++----
>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>> 
>>>> diff --git a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>> index 290f2a1..3e4eff9 100644
>>>> --- a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>> +++ b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>> @@ -2741,24 +2741,24 @@ SDValue DAGCombiner::visitAND(SDNode *N) {
>>>>     ISD::CondCode Op0 = cast<CondCodeSDNode>(CC0)->get();
>>>>     ISD::CondCode Op1 = cast<CondCodeSDNode>(CC1)->get();
>>>> 
>>>> -    if (LR == RR && isa<ConstantSDNode>(LR) && Op0 == Op1 &&
>>>> +    if (LR == RR && Op0 == Op1 &&
>>>>         LL.getValueType().isInteger()) {
>>>>       // fold (and (seteq X, 0), (seteq Y, 0)) -> (seteq (or X, Y), 0)
>>>> -      if (cast<ConstantSDNode>(LR)->isNullValue() && Op1 == ISD::SETEQ) {
>>>> +      if (TLI.isConstFalseVal(LR.getNode()) && Op1 == ISD::SETEQ) {
>>>>         SDValue ORNode = DAG.getNode(ISD::OR, SDLoc(N0),
>>>>                                      LR.getValueType(), LL, RL);
>>>>         AddToWorkList(ORNode.getNode());
>>>>         return DAG.getSetCC(SDLoc(N), VT, ORNode, LR, Op1);
>>>>       }
>>>>       // fold (and (seteq X, -1), (seteq Y, -1)) -> (seteq (and X, Y), -1)
>>>> -      if (cast<ConstantSDNode>(LR)->isAllOnesValue() && Op1 == ISD::SETEQ) {
>>>> +      if (TLI.isConstTrueVal(LR.getNode()) && Op1 == ISD::SETEQ) {
>>>>         SDValue ANDNode = DAG.getNode(ISD::AND, SDLoc(N0),
>>>>                                       LR.getValueType(), LL, RL);
>>>>         AddToWorkList(ANDNode.getNode());
>>>>         return DAG.getSetCC(SDLoc(N), VT, ANDNode, LR, Op1);
>>>>       }
>>>>       // fold (and (setgt X,  -1), (setgt Y,  -1)) -> (setgt (or X, Y), -1)
>>>> -      if (cast<ConstantSDNode>(LR)->isAllOnesValue() && Op1 == ISD::SETGT) {
>>>> +      if (TLI.isConstTrueVal(LR.getNode()) && Op1 == ISD::SETGT) {
>>>>         SDValue ORNode = DAG.getNode(ISD::OR, SDLoc(N0),
>>>>                                      LR.getValueType(), LL, RL);
>>>>         AddToWorkList(ORNode.getNode());
>>>> -- 
>>>> 1.8.1.5
>>>> 
>>> 
>>>> From c4ac3c7724bfdcf71349808019d2d4f8835fe1ad Mon Sep 17 00:00:00 2001
>>>> From: Tom Stellard <thomas.stellard at amd.com>
>>>> Date: Fri, 2 May 2014 22:31:06 -0400
>>>> Subject: [PATCH 4/4] SelectionDAG: Don't use MVT::Other to determine legality
>>>> of ISD::SELECT_CC
>>>> 
>>>> The SelectionDAG bad a special case for ISD::SELECT_CC, where it would
>>>> allow targets to specify:
>>>> 
>>>> setOperationAction(ISD::SELECT_CC, MVT::Other, Expand);
>>>> 
>>>> to indicate that they wanted to expand ISD::SELECT_CC for all types.
>>>> This wasn't applied correctly everywhere, and it makes writing new
>>>> DAG patterns with ISD::SELECT_CC difficult.
>>>> ---
>>>> lib/CodeGen/SelectionDAG/DAGCombiner.cpp   | 21 +++++----------------
>>>> lib/Target/Hexagon/HexagonISelLowering.cpp |  6 ------
>>>> lib/Target/Mips/MipsISelLowering.cpp       |  3 ++-
>>>> lib/Target/NVPTX/NVPTXISelLowering.cpp     |  8 +++++++-
>>>> lib/Target/X86/X86ISelLowering.cpp         |  9 ++++++++-
>>>> lib/Target/XCore/XCoreISelLowering.cpp     |  3 ---
>>>> test/CodeGen/Mips/selectcc.ll              |  8 +++-----
>>>> 7 files changed, 25 insertions(+), 33 deletions(-)
>>>> 
>>>> diff --git a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>> index 3e4eff9..b0e5e8d 100644
>>>> --- a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>> +++ b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>>> @@ -4550,12 +4550,9 @@ SDValue DAGCombiner::visitSELECT(SDNode *N) {
>>>> 
>>>>   // fold selects based on a setcc into other things, such as min/max/abs
>>>>   if (N0.getOpcode() == ISD::SETCC) {
>>>> -    // FIXME:
>>>> -    // Check against MVT::Other for SELECT_CC, which is a workaround for targets
>>>> -    // having to say they don't support SELECT_CC on every type the DAG knows
>>>> -    // about, since there is no way to mark an opcode illegal at all value types
>>>> -    if (TLI.isOperationLegalOrCustom(ISD::SELECT_CC, MVT::Other) &&
>>>> -        TLI.isOperationLegalOrCustom(ISD::SELECT_CC, VT))
>>>> +    if ((!LegalOperations &&
>>>> +         TLI.isOperationLegalOrCustom(ISD::SELECT_CC, VT)) ||
>>>> +	TLI.isOperationLegal(ISD::SELECT_CC, VT))
>>>>       return DAG.getNode(ISD::SELECT_CC, SDLoc(N), VT,
>>>>                          N0.getOperand(0), N0.getOperand(1),
>>>>                          N1, N2, N0.getOperand(2));
>>>> @@ -6950,11 +6947,7 @@ SDValue DAGCombiner::visitSINT_TO_FP(SDNode *N) {
>>>>   }
>>>> 
>>>>   // The next optimizations are desirable only if SELECT_CC can be lowered.
>>>> -  // Check against MVT::Other for SELECT_CC, which is a workaround for targets
>>>> -  // having to say they don't support SELECT_CC on every type the DAG knows
>>>> -  // about, since there is no way to mark an opcode illegal at all value types
>>>> -  // (See also visitSELECT)
>>>> -  if (TLI.isOperationLegalOrCustom(ISD::SELECT_CC, MVT::Other)) {
>>>> +  if (TLI.isOperationLegalOrCustom(ISD::SELECT_CC, VT) || !LegalOperations) {
>>>>     // fold (sint_to_fp (setcc x, y, cc)) -> (select_cc x, y, -1.0, 0.0,, cc)
>>>>     if (N0.getOpcode() == ISD::SETCC && N0.getValueType() == MVT::i1 &&
>>>>         !VT.isVector() &&
>>>> @@ -7007,11 +7000,7 @@ SDValue DAGCombiner::visitUINT_TO_FP(SDNode *N) {
>>>>   }
>>>> 
>>>>   // The next optimizations are desirable only if SELECT_CC can be lowered.
>>>> -  // Check against MVT::Other for SELECT_CC, which is a workaround for targets
>>>> -  // having to say they don't support SELECT_CC on every type the DAG knows
>>>> -  // about, since there is no way to mark an opcode illegal at all value types
>>>> -  // (See also visitSELECT)
>>>> -  if (TLI.isOperationLegalOrCustom(ISD::SELECT_CC, MVT::Other)) {
>>>> +  if (TLI.isOperationLegalOrCustom(ISD::SELECT_CC, VT) || !LegalOperations) {
>>>>     // fold (uint_to_fp (setcc x, y, cc)) -> (select_cc x, y, -1.0, 0.0,, cc)
>>>> 
>>>>     if (N0.getOpcode() == ISD::SETCC && !VT.isVector() &&
>>>> diff --git a/lib/Target/Hexagon/HexagonISelLowering.cpp b/lib/Target/Hexagon/HexagonISelLowering.cpp
>>>> index aa4e9b5..394a552 100644
>>>> --- a/lib/Target/Hexagon/HexagonISelLowering.cpp
>>>> +++ b/lib/Target/Hexagon/HexagonISelLowering.cpp
>>>> @@ -1339,18 +1339,12 @@ HexagonTargetLowering::HexagonTargetLowering(HexagonTargetMachine
>>>> 
>>>>       setOperationAction(ISD::SELECT_CC, MVT::f32, Expand);
>>>>       setOperationAction(ISD::SELECT_CC, MVT::f64, Expand);
>>>> -      setOperationAction(ISD::SELECT_CC, MVT::Other, Expand);
>>>> 
>>>>     } else {
>>>> 
>>>>       // Hexagon has no select or setcc: expand to SELECT_CC.
>>>>       setOperationAction(ISD::SELECT, MVT::f32, Expand);
>>>>       setOperationAction(ISD::SELECT, MVT::f64, Expand);
>>>> -
>>>> -      // This is a workaround documented in DAGCombiner.cpp:2892 We don't
>>>> -      // support SELECT_CC on every type.
>>>> -      setOperationAction(ISD::SELECT_CC, MVT::Other,   Expand);
>>>> -
>>>>     }
>>>> 
>>>>     if (EmitJumpTables) {
>>>> diff --git a/lib/Target/Mips/MipsISelLowering.cpp b/lib/Target/Mips/MipsISelLowering.cpp
>>>> index d152e67..9dcc772 100644
>>>> --- a/lib/Target/Mips/MipsISelLowering.cpp
>>>> +++ b/lib/Target/Mips/MipsISelLowering.cpp
>>>> @@ -287,7 +287,8 @@ MipsTargetLowering::MipsTargetLowering(MipsTargetMachine &TM)
>>>>   setOperationAction(ISD::BR_CC,             MVT::f64,   Expand);
>>>>   setOperationAction(ISD::BR_CC,             MVT::i32,   Expand);
>>>>   setOperationAction(ISD::BR_CC,             MVT::i64,   Expand);
>>>> -  setOperationAction(ISD::SELECT_CC,         MVT::Other, Expand);
>>>> +  setOperationAction(ISD::SELECT_CC,         MVT::i32,   Expand);
>>>> +  setOperationAction(ISD::SELECT_CC,         MVT::i64,   Expand);
>>>>   setOperationAction(ISD::UINT_TO_FP,        MVT::i32,   Expand);
>>>>   setOperationAction(ISD::UINT_TO_FP,        MVT::i64,   Expand);
>>>>   setOperationAction(ISD::FP_TO_UINT,        MVT::i32,   Expand);
>>>> diff --git a/lib/Target/NVPTX/NVPTXISelLowering.cpp b/lib/Target/NVPTX/NVPTXISelLowering.cpp
>>>> index e6860a9..91e6dac 100644
>>>> --- a/lib/Target/NVPTX/NVPTXISelLowering.cpp
>>>> +++ b/lib/Target/NVPTX/NVPTXISelLowering.cpp
>>>> @@ -130,7 +130,13 @@ NVPTXTargetLowering::NVPTXTargetLowering(NVPTXTargetMachine &TM)
>>>>   addRegisterClass(MVT::f64, &NVPTX::Float64RegsRegClass);
>>>> 
>>>>   // Operations not directly supported by NVPTX.
>>>> -  setOperationAction(ISD::SELECT_CC, MVT::Other, Expand);
>>>> +  setOperationAction(ISD::SELECT_CC, MVT::f32, Expand);
>>>> +  setOperationAction(ISD::SELECT_CC, MVT::f64, Expand);
>>>> +  setOperationAction(ISD::SELECT_CC, MVT::i1, Expand);
>>>> +  setOperationAction(ISD::SELECT_CC, MVT::i8, Expand);
>>>> +  setOperationAction(ISD::SELECT_CC, MVT::i16, Expand);
>>>> +  setOperationAction(ISD::SELECT_CC, MVT::i32, Expand);
>>>> +  setOperationAction(ISD::SELECT_CC, MVT::i64, Expand);
>>>>   setOperationAction(ISD::BR_CC, MVT::f32, Expand);
>>>>   setOperationAction(ISD::BR_CC, MVT::f64, Expand);
>>>>   setOperationAction(ISD::BR_CC, MVT::i1, Expand);
>>>> diff --git a/lib/Target/X86/X86ISelLowering.cpp b/lib/Target/X86/X86ISelLowering.cpp
>>>> index 0ed30f5..0362bce 100644
>>>> --- a/lib/Target/X86/X86ISelLowering.cpp
>>>> +++ b/lib/Target/X86/X86ISelLowering.cpp
>>>> @@ -442,7 +442,13 @@ void X86TargetLowering::resetOperationActions() {
>>>>   setOperationAction(ISD::BR_CC            , MVT::i16,   Expand);
>>>>   setOperationAction(ISD::BR_CC            , MVT::i32,   Expand);
>>>>   setOperationAction(ISD::BR_CC            , MVT::i64,   Expand);
>>>> -  setOperationAction(ISD::SELECT_CC        , MVT::Other, Expand);
>>>> +  setOperationAction(ISD::SELECT_CC        , MVT::f32,   Expand);
>>>> +  setOperationAction(ISD::SELECT_CC        , MVT::f64,   Expand);
>>>> +  setOperationAction(ISD::SELECT_CC        , MVT::f80,   Expand);
>>>> +  setOperationAction(ISD::SELECT_CC        , MVT::i8,    Expand);
>>>> +  setOperationAction(ISD::SELECT_CC        , MVT::i16,   Expand);
>>>> +  setOperationAction(ISD::SELECT_CC        , MVT::i32,   Expand);
>>>> +  setOperationAction(ISD::SELECT_CC        , MVT::i64,   Expand);
>>>>   if (Subtarget->is64Bit())
>>>>     setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i32, Legal);
>>>>   setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i16  , Legal);
>>>> @@ -860,6 +866,7 @@ void X86TargetLowering::resetOperationActions() {
>>>>     setOperationAction(ISD::ZERO_EXTEND, VT, Expand);
>>>>     setOperationAction(ISD::ANY_EXTEND, VT, Expand);
>>>>     setOperationAction(ISD::VSELECT, VT, Expand);
>>>> +    setOperationAction(ISD::SELECT_CC, VT, Expand);
>>>>     for (int InnerVT = MVT::FIRST_VECTOR_VALUETYPE;
>>>>              InnerVT <= MVT::LAST_VECTOR_VALUETYPE; ++InnerVT)
>>>>       setTruncStoreAction(VT,
>>>> diff --git a/lib/Target/XCore/XCoreISelLowering.cpp b/lib/Target/XCore/XCoreISelLowering.cpp
>>>> index 96eebb4..37f53c5 100644
>>>> --- a/lib/Target/XCore/XCoreISelLowering.cpp
>>>> +++ b/lib/Target/XCore/XCoreISelLowering.cpp
>>>> @@ -98,9 +98,6 @@ XCoreTargetLowering::XCoreTargetLowering(XCoreTargetMachine &XTM)
>>>>   setOperationAction(ISD::SUBC, MVT::i32, Expand);
>>>>   setOperationAction(ISD::SUBE, MVT::i32, Expand);
>>>> 
>>>> -  // Stop the combiner recombining select and set_cc
>>>> -  setOperationAction(ISD::SELECT_CC, MVT::Other, Expand);
>>>> -
>>>>   // 64bit
>>>>   setOperationAction(ISD::ADD, MVT::i64, Custom);
>>>>   setOperationAction(ISD::SUB, MVT::i64, Custom);
>>>> diff --git a/test/CodeGen/Mips/selectcc.ll b/test/CodeGen/Mips/selectcc.ll
>>>> index aeef60e..626fb95 100644
>>>> --- a/test/CodeGen/Mips/selectcc.ll
>>>> +++ b/test/CodeGen/Mips/selectcc.ll
>>>> @@ -16,13 +16,11 @@ entry:
>>>> ; SOURCE-SCHED: lw
>>>> ; SOURCE-SCHED: lui
>>>> ; SOURCE-SCHED: sw
>>>> -; SOURCE-SCHED: addiu
>>>> -; SOURCE-SCHED: addiu
>>>> -; SOURCE-SCHED: c.olt.s
>>>> -; SOURCE-SCHED: movt
>>>> +; SOURCE-SCHED: lw
>>>> +; SOURCE-SCHED: lwc1
>>>> ; SOURCE-SCHED: mtc1
>>>> +; SOURCE-SCHED: c.olt.s
>>>> ; SOURCE-SCHED: jr
>>>> -
>>>>   store float 0.000000e+00, float* @gf0, align 4
>>>>   store float 1.000000e+00, float* @gf1, align 4
>>>>   %cmp = fcmp olt float %a, %b
>>>> -- 
>>>> 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
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

More information about the llvm-commits mailing list