[llvm] r194542 - SelectionDAG: Teach the legalizer to split SETCC if VSELECT needs splitting too.
Juergen Ributzka
juergen at apple.com
Mon Nov 18 17:51:23 PST 2013
Hi Tom,
sorry for not coming back to you earlier. I was working on a patch for this problem this weekend, but then I stumbled across another issue that was exposed while working on the patch. The two pending patches for SelectionDAG that I posted to the mailing list are in preparation for fixing this issue. I looked at your second patch and using SplitVecRes_SETCC would fix the problem, but it also splits the SETCC result vector again, instead of using the already split vectors provided by GetSplitVector. GetSplitVector is just a cache of already split vectors for that given operation. I would prefer not to duplicate split vector nodes if they already exist. The issue for your backend is that split never occurred and that is why the assertion is failing. I hope to get this fixed for you soon.
Cheers,
Juergen
On Nov 18, 2013, at 4:49 PM, Tom Stellard <tom at stellard.net> wrote:
> 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>
>>
> <0001-R600-Fix-getSetCCResultType-to-handle-new-VSELECT-DA.patch><0002-SelectionDAG-Fix-assertion-failure-when-splitting-ve.patch>
More information about the llvm-commits
mailing list