PATCH: SelectionDAG: Remove setOperationAction(ISD::SELECT_CC, MVT::Other, Expand) hack.
Tom Stellard
tom at stellard.net
Fri May 9 07:16:12 PDT 2014
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
More information about the llvm-commits
mailing list