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