[llvm] r194542 - SelectionDAG: Teach the legalizer to split SETCC if VSELECT needs splitting too.
Tom Stellard
tom at stellard.net
Mon Nov 18 16:49:01 PST 2013
Hi Juergen,
These two patches fix the crash for me. The first one is an R600 fix
and the second is a fix for illegal SELECT with legal SETCC as its
condition. I'm not so sure patch number 2 is the right way to
fix the bug, but it was the most simple fix I could come up with.
-Tom
On Fri, Nov 15, 2013 at 10:45:11AM -0800, Juergen Ributzka wrote:
> Hi Tom,
>
> I took a quick look yesterday night and I didn’t see a quick fix either. I will look into it today.
>
> Cheers,
> Juergen
>
>
> On Nov 14, 2013, at 8:45 PM, Tom Stellard <tom at stellard.net> wrote:
>
> > On Wed, Nov 13, 2013 at 01:57:54AM -0000, Juergen Ributzka wrote:
> >> Author: ributzka
> >> Date: Tue Nov 12 19:57:54 2013
> >> New Revision: 194542
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=194542&view=rev
> >> Log:
> >> SelectionDAG: Teach the legalizer to split SETCC if VSELECT needs splitting too.
> >>
> >> This patch reapplies r193676 with an additional fix for the Hexagon backend. The
> >> SystemZ backend has already been fixed by r194148.
> >>
> >> The Type Legalizer recognizes that VSELECT needs to be split, because the type
> >> is to wide for the given target. The same does not always apply to SETCC,
> >> because less space is required to encode the result of a comparison. As a result
> >> VSELECT is split and SETCC is unrolled into scalar comparisons.
> >>
> >> This commit fixes the issue by checking for VSELECT-SETCC patterns in the DAG
> >> Combiner. If a matching pattern is found, then the result mask of SETCC is
> >> promoted to the expected vector mask type for the given target. Now the type
> >> legalizer will split both VSELECT and SETCC.
> >>
> >> This allows the following X86 DAG Combine code to sucessfully detect the MIN/MAX
> >> pattern. This fixes PR16695, PR17002, and <rdar://problem/14594431>.
> >>
> >> Reviewed by Nadav
> >>
> >
> > Hi Juergen,
> >
> > This caused a regression on R600. I have attached a patch to this email
> > with a testcase. See my comments below.
> >
> >> Added:
> >> llvm/trunk/test/CodeGen/X86/vec_split.ll
> >> Modified:
> >> llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> >> llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypesGeneric.cpp
> >> llvm/trunk/lib/Target/Hexagon/HexagonISelLowering.h
> >> llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
> >>
> >> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> >> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=194542&r1=194541&r2=194542&view=diff
> >> ==============================================================================
> >> --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
> >> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Tue Nov 12 19:57:54 2013
> >> @@ -4364,6 +4364,29 @@ SDValue DAGCombiner::visitVSELECT(SDNode
> >> }
> >> }
> >>
> >> + // Treat SETCC as a vector mask and promote the result type based on the
> >> + // targets expected SETCC result type. This will ensure that SETCC and VSELECT
> >> + // are both split by the type legalizer. This is done to prevent the type
> >> + // legalizer from unrolling SETCC into scalar comparions.
> >> + EVT SelectVT = N->getValueType(0);
> >> + EVT MaskVT = getSetCCResultType(SelectVT);
> >> + assert(MaskVT.isVector() && "Expected a vector type.");
> >> + if (N0.getOpcode() == ISD::SETCC && N0.getValueType() != MaskVT) {
> >> + SDLoc MaskDL(N0);
> >> +
> >> + // Extend the mask to the desired value type.
> >> + ISD::NodeType ExtendCode =
> >> + TargetLowering::getExtendForContent(TLI.getBooleanContents(true));
> >> + SDValue Mask = DAG.getNode(ExtendCode, MaskDL, MaskVT, N0);
> >> +
> >> + AddToWorkList(Mask.getNode());
> >> +
> >> + SDValue LHS = N->getOperand(1);
> >> + SDValue RHS = N->getOperand(2);
> >> +
> >> + return DAG.getNode(ISD::VSELECT, DL, SelectVT, Mask, LHS, RHS);
> >> + }
> >> +
> >> return SDValue();
> >> }
> >>
> >>
> >> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypesGeneric.cpp
> >> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypesGeneric.cpp?rev=194542&r1=194541&r2=194542&view=diff
> >> ==============================================================================
> >> --- llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypesGeneric.cpp (original)
> >> +++ llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeTypesGeneric.cpp Tue Nov 12 19:57:54 2013
> >> @@ -492,14 +492,19 @@ void DAGTypeLegalizer::SplitRes_SELECT(S
> >> SDValue Cond = N->getOperand(0);
> >> CL = CH = Cond;
> >> if (Cond.getValueType().isVector()) {
> >> - assert(Cond.getValueType().getVectorElementType() == MVT::i1 &&
> >> - "Condition legalized before result?");
> >> - unsigned NumElements = Cond.getValueType().getVectorNumElements();
> >> - EVT VCondTy = EVT::getVectorVT(*DAG.getContext(), MVT::i1, NumElements / 2);
> >> - CL = DAG.getNode(ISD::EXTRACT_SUBVECTOR, dl, VCondTy, Cond,
> >> - DAG.getConstant(0, TLI.getVectorIdxTy()));
> >> - CH = DAG.getNode(ISD::EXTRACT_SUBVECTOR, dl, VCondTy, Cond,
> >> - DAG.getConstant(NumElements / 2, TLI.getVectorIdxTy()));
> >> + if (Cond.getOpcode() == ISD::SETCC) {
> >> + assert(Cond.getValueType() == getSetCCResultType(N->getValueType(0)) &&
> >> + "Condition has not been prepared for split!");
> >> + GetSplitVector(Cond, CL, CH);
> >
> > Even with the adjustment to getSetCCResultType in the attached patch,
> > when GetSplitVector() is called here, I am getting an assertion failure:
> >
> > llc: LegalizeTypes.cpp:828: void llvm::DAGTypeLegalizer::GetSplitVector(llvm::SDValue, llvm::SDValue &, llvm::SDValue &): Assertion `Entry.first.getNode() && "Operand isn't split"' failed.
> >
> > From what I can tell the problem here is that 'Cond' has a value type
> > of v4i32 which is legal on R600, so the operation was not split, and
> > therefore there is no entry in the SplitVectors map for it (This is what
> > is causing the assertion failure).
> >
> > The SELECT in this example has a type of v4i64 which is not a legal type
> > on R600, so the problem occurs when a SELECT node has an illegal type,
> > but Operand 0 is a SETCC node with a legal type. I'm not sure the best
> > way to fix this issue. Do you have any suggestions?
> >
> > Thanks,
> > Tom
> >
> >
> >> + } else {
> >> + EVT ETy = Cond.getValueType().getVectorElementType();
> >> + unsigned NumElements = Cond.getValueType().getVectorNumElements();
> >> + EVT VCondTy = EVT::getVectorVT(*DAG.getContext(), ETy, NumElements / 2);
> >> + CL = DAG.getNode(ISD::EXTRACT_SUBVECTOR, dl, VCondTy, Cond,
> >> + DAG.getConstant(0, TLI.getVectorIdxTy()));
> >> + CH = DAG.getNode(ISD::EXTRACT_SUBVECTOR, dl, VCondTy, Cond,
> >> + DAG.getConstant(NumElements / 2, TLI.getVectorIdxTy()));
> >> + }
> >> }
> >>
> >> Lo = DAG.getNode(N->getOpcode(), dl, LL.getValueType(), CL, LL, RL);
> >>
> >> Modified: llvm/trunk/lib/Target/Hexagon/HexagonISelLowering.h
> >> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Hexagon/HexagonISelLowering.h?rev=194542&r1=194541&r2=194542&view=diff
> >> ==============================================================================
> >> --- llvm/trunk/lib/Target/Hexagon/HexagonISelLowering.h (original)
> >> +++ llvm/trunk/lib/Target/Hexagon/HexagonISelLowering.h Tue Nov 12 19:57:54 2013
> >> @@ -141,8 +141,11 @@ namespace llvm {
> >>
> >> SDValue LowerVASTART(SDValue Op, SelectionDAG &DAG) const;
> >> SDValue LowerConstantPool(SDValue Op, SelectionDAG &DAG) const;
> >> - virtual EVT getSetCCResultType(LLVMContext &, EVT) const {
> >> - return MVT::i1;
> >> + virtual EVT getSetCCResultType(LLVMContext &C, EVT VT) const {
> >> + if (!VT.isVector())
> >> + return MVT::i1;
> >> + else
> >> + return EVT::getVectorVT(C, MVT::i1, VT.getVectorNumElements());
> >> }
> >>
> >> virtual bool getPostIndexedAddressParts(SDNode *N, SDNode *Op,
> >>
> >> Modified: llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
> >> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.cpp?rev=194542&r1=194541&r2=194542&view=diff
> >> ==============================================================================
> >> --- llvm/trunk/lib/Target/X86/X86ISelLowering.cpp (original)
> >> +++ llvm/trunk/lib/Target/X86/X86ISelLowering.cpp Tue Nov 12 19:57:54 2013
> >> @@ -1547,7 +1547,16 @@ void X86TargetLowering::resetOperationAc
> >> }
> >>
> >> EVT X86TargetLowering::getSetCCResultType(LLVMContext &, EVT VT) const {
> >> - if (!VT.isVector()) return MVT::i8;
> >> + if (!VT.isVector())
> >> + return MVT::i8;
> >> +
> >> + const TargetMachine &TM = getTargetMachine();
> >> + if (!TM.Options.UseSoftFloat && Subtarget->hasAVX512())
> >> + switch(VT.getVectorNumElements()) {
> >> + case 8: return MVT::v8i1;
> >> + case 16: return MVT::v16i1;
> >> + }
> >> +
> >> return VT.changeVectorElementTypeToInteger();
> >> }
> >>
> >>
> >> Added: llvm/trunk/test/CodeGen/X86/vec_split.ll
> >> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/vec_split.ll?rev=194542&view=auto
> >> ==============================================================================
> >> --- llvm/trunk/test/CodeGen/X86/vec_split.ll (added)
> >> +++ llvm/trunk/test/CodeGen/X86/vec_split.ll Tue Nov 12 19:57:54 2013
> >> @@ -0,0 +1,42 @@
> >> +; RUN: llc -march=x86-64 -mcpu=corei7 < %s | FileCheck %s -check-prefix=SSE4
> >> +; RUN: llc -march=x86-64 -mcpu=corei7-avx < %s | FileCheck %s -check-prefix=AVX1
> >> +; RUN: llc -march=x86-64 -mcpu=core-avx2 < %s | FileCheck %s -check-prefix=AVX2
> >> +
> >> +define <16 x i16> @split16(<16 x i16> %a, <16 x i16> %b, <16 x i8> %__mask) {
> >> +; SSE4-LABEL: split16:
> >> +; SSE4: pminuw
> >> +; SSE4: pminuw
> >> +; SSE4: ret
> >> +; AVX1-LABEL: split16:
> >> +; AVX1: vpminuw
> >> +; AVX1: vpminuw
> >> +; AVX1: ret
> >> +; AVX2-LABEL: split16:
> >> +; AVX2: vpminuw
> >> +; AVX2: ret
> >> + %1 = icmp ult <16 x i16> %a, %b
> >> + %2 = select <16 x i1> %1, <16 x i16> %a, <16 x i16> %b
> >> + ret <16 x i16> %2
> >> +}
> >> +
> >> +define <32 x i16> @split32(<32 x i16> %a, <32 x i16> %b, <32 x i8> %__mask) {
> >> +; SSE4-LABEL: split32:
> >> +; SSE4: pminuw
> >> +; SSE4: pminuw
> >> +; SSE4: pminuw
> >> +; SSE4: pminuw
> >> +; SSE4: ret
> >> +; AVX1-LABEL: split32:
> >> +; AVX1: vpminuw
> >> +; AVX1: vpminuw
> >> +; AVX1: vpminuw
> >> +; AVX1: vpminuw
> >> +; AVX1: ret
> >> +; AVX2-LABEL: split32:
> >> +; AVX2: vpminuw
> >> +; AVX2: vpminuw
> >> +; AVX2: ret
> >> + %1 = icmp ult <32 x i16> %a, %b
> >> + %2 = select <32 x i1> %1, <32 x i16> %a, <32 x i16> %b
> >> + ret <32 x i16> %2
> >> +}
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> > <0001-R600-Fix-getSetCCResultType-to-handle-new-VSELECT-DA.patch>
>
-------------- next part --------------
>From bbabd455f7c4d8446d64543383520cc248c1e204 Mon Sep 17 00:00:00 2001
From: Tom Stellard <thomas.stellard at amd.com>
Date: Thu, 14 Nov 2013 18:52:45 -0800
Subject: [PATCH 1/2] R600: Fix getSetCCResultType() to handle new VSELECT DAG
Combine
This fixes a crash introduced by r194542.
No test case yet, because the test case is still crashing due to
a separate bug.
---
lib/Target/R600/R600ISelLowering.cpp | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/lib/Target/R600/R600ISelLowering.cpp b/lib/Target/R600/R600ISelLowering.cpp
index 0fcb488..b174fe1 100644
--- a/lib/Target/R600/R600ISelLowering.cpp
+++ b/lib/Target/R600/R600ISelLowering.cpp
@@ -1383,9 +1383,11 @@ SDValue R600TargetLowering::LowerFormalArguments(
return Chain;
}
-EVT R600TargetLowering::getSetCCResultType(LLVMContext &, EVT VT) const {
- if (!VT.isVector()) return MVT::i32;
- return VT.changeVectorElementTypeToInteger();
+EVT R600TargetLowering::getSetCCResultType(LLVMContext &C, EVT VT) const {
+ if (!VT.isVector())
+ return MVT::i32;
+ else
+ return EVT::getVectorVT(C, MVT::i32, VT.getVectorNumElements());
}
static SDValue
--
1.8.1.4
-------------- next part --------------
>From bafb505052fdec597185f681abe9639c596477c2 Mon Sep 17 00:00:00 2001
From: Tom Stellard <thomas.stellard at amd.com>
Date: Mon, 18 Nov 2013 16:17:00 -0800
Subject: [PATCH 2/2] SelectionDAG: Fix assertion failure when splitting vector
SELECT ops
This fixes an assertion in the DAG Type Legalizer that occurs when
the Legalizer attempts to split a vector SELECT node that has
a SETCC with a legal type as its 'condition' operand.
This behavior leading to assertion failure was introduced by r194542.
---
lib/CodeGen/SelectionDAG/LegalizeTypesGeneric.cpp | 2 +-
test/CodeGen/R600/vselect.ll | 10 ++++++++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/lib/CodeGen/SelectionDAG/LegalizeTypesGeneric.cpp b/lib/CodeGen/SelectionDAG/LegalizeTypesGeneric.cpp
index f1b06fc..8bef239 100644
--- a/lib/CodeGen/SelectionDAG/LegalizeTypesGeneric.cpp
+++ b/lib/CodeGen/SelectionDAG/LegalizeTypesGeneric.cpp
@@ -495,7 +495,7 @@ void DAGTypeLegalizer::SplitRes_SELECT(SDNode *N, SDValue &Lo,
if (Cond.getOpcode() == ISD::SETCC) {
assert(Cond.getValueType() == getSetCCResultType(N->getValueType(0)) &&
"Condition has not been prepared for split!");
- GetSplitVector(Cond, CL, CH);
+ SplitVecRes_SETCC(Cond.getNode(), CL, CH);
} else {
EVT ETy = Cond.getValueType().getVectorElementType();
unsigned NumElements = Cond.getValueType().getVectorNumElements();
diff --git a/test/CodeGen/R600/vselect.ll b/test/CodeGen/R600/vselect.ll
index dca7b06..5d7cef4 100644
--- a/test/CodeGen/R600/vselect.ll
+++ b/test/CodeGen/R600/vselect.ll
@@ -74,3 +74,13 @@ entry:
store <4 x float> %result, <4 x float> addrspace(1)* %out
ret void
}
+
+; EG-CHECK-LABEL: @test_select_v4i64
+
+define void @test_select_v4i64(<4 x i64> addrspace(1)* %out, <4 x i32> %c) {
+entry:
+ %cmp = icmp ne <4 x i32> %c, <i32 0, i32 0, i32 0, i32 0>
+ %result = select <4 x i1> %cmp, <4 x i64> <i64 0, i64 1, i64 2, i64 3>, <4 x i64> <i64 4, i64 5, i64 6, i64 7>
+ store <4 x i64> %result, <4 x i64> addrspace(1)* %out
+ ret void
+}
--
1.8.1.4
More information about the llvm-commits
mailing list