PATCH: SelectionDAG: Remove setOperationAction(ISD::SELECT_CC, MVT::Other, Expand) hack.
Tom Stellard
tom at stellard.net
Sun May 25 09:10:02 PDT 2014
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