[llvm] r311892 - [DAGCombiner] Teach visitEXTRACT_SUBVECTOR to turn extracts of BUILD_VECTOR into smaller BUILD_VECTORs

Justin Bogner via llvm-commits llvm-commits at lists.llvm.org
Wed Sep 6 14:59:32 PDT 2017


Craig Topper <craig.topper at gmail.com> writes:
> I implemented your suggestion in r312621. Let me know if it doesn't fix
> your issue.

Looks like it works. Thanks!

> ~Craig
>
> On Tue, Sep 5, 2017 at 1:54 PM, Justin Bogner via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> "Topper, Craig via llvm-commits" <llvm-commits at lists.llvm.org> writes:
>> > Hopefully fixed with r312253. My attempt at preventing element splits
>> > wasn’t strong enough.
>>
>> It looks like this fixed the case that was reported here, but I'm still
>> seeing a related problem where we get something like the below (using
>> similar debug output to escha's example):
>>
>>   Combining: t95: v2i8 = extract_subvector t74, Constant:i32<0>
>>   ---------
>>   t74: v4i8 = bitcast t73
>>   t73: v2i16 = BUILD_VECTOR t70, t72
>>   ExtractSize: 16, EltSize: 16, NumElems: 1
>>   LegalOperations: 0
>>   LegalTypes:      1
>>   isLegalOp:       0
>>   t151: v1i16 = BUILD_VECTOR t70
>>   ---------
>>    ... into: t152: v2i8 = bitcast t151
>>
>> Here we create a v1i16 (which is not a legal type), but we're past type
>> legalization where we would scalarize this.
>>
>> I guess the easiest fix is to check LegalTypes/isTypeLegal much like we
>> do with the operation already:
>>
>> diff --git a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> index ae923e8ac07..c068e1df836 100644
>> --- a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> +++ b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> @@ -15180,8 +15180,9 @@ SDValue DAGCombiner::visitEXTRACT_SUBVECTOR(SDNode*
>> N) {
>>          unsigned NumElems = ExtractSize / EltSize;
>>          EVT ExtractVT = EVT::getVectorVT(*DAG.getContext(),
>>                                           InVT.getVectorElementType(),
>> NumElems);
>> -        if (!LegalOperations ||
>> -            TLI.isOperationLegal(ISD::BUILD_VECTOR, ExtractVT)) {
>> +        if ((!LegalOperations ||
>> +             TLI.isOperationLegal(ISD::BUILD_VECTOR, ExtractVT)) &&
>> +            (!LegalTypes || TLI.isTypeLegal(ExtractVT))) {
>>            unsigned IdxVal = (Idx->getZExtValue() *
>> NVT.getScalarSizeInBits()) /
>>                              EltSize;
>>
>> WDYT?
>>
>> > From: escha at apple.com
>> > Sent: Thursday, August 31, 2017 1:41 AM
>> > To: Topper, Craig <craig.topper at intel.com>
>> > Cc: llvm-commits at lists.llvm.org
>> > Subject: Re: [llvm] r311892 - [DAGCombiner] Teach visitEXTRACT_SUBVECTOR
>> to turn extracts of BUILD_VECTOR into smaller BUILD_VECTORs
>> >
>> > Here’s what it looks like.
>> >
>> > I added debug statements as follows:
>> >
>> >   fprintf(stderr,"foo\n");
>> >   V.dump();
>> >   // Skip bitcasting
>> >   V = peekThroughBitcast(V);
>> >   V.dump();
>> >
>> >   // If the input is a build vector. Try to make a smaller build vector.
>> >   if (V->getOpcode() == ISD::BUILD_VECTOR) {
>> >     if (auto *Idx = dyn_cast<ConstantSDNode>(N->getOperand(1))) {
>> >       EVT InVT = V->getValueType(0);
>> >       unsigned NumElems = NVT.getSizeInBits() /
>> InVT.getScalarSizeInBits();
>> >       fprintf(stderr,"%d %d %d\n", NVT.getSizeInBits(),
>> InVT.getScalarSizeInBits(), NumElems);
>> >       if (NumElems > 0) {
>> >         EVT ExtractVT = EVT::getVectorVT(*DAG.getContext(),
>> >                                          InVT.getVectorElementType(),
>> NumElems);
>> >         if (!LegalOperations ||
>> >             TLI.isOperationLegal(ISD::BUILD_VECTOR, ExtractVT)) {
>> >           unsigned IdxVal = Idx->getZExtValue() *
>> NVT.getScalarSizeInBits() /
>> >                             InVT.getScalarSizeInBits();
>> >           fprintf(stderr,"%d\n", IdxVal);
>> >
>> >           // Extract the pieces from the original build_vector.
>> >           SDValue BuildVec = DAG.getBuildVector(ExtractVT, SDLoc(N),
>> >                                             makeArrayRef(V->op_begin() +
>> IdxVal,
>> >                                                          NumElems));
>> >           BuildVec.dump();
>> >           return DAG.getBitcast(NVT, BuildVec);
>> >         }
>> >       }
>> >     }
>> >   }
>> >
>> > This results in the following printout
>> >
>> > Combining: t12: v3f16 = extract_subvector t168, Constant:i32<0>
>> > foo
>> > t168: v4f16 = bitcast t167
>> > t167: v2f32 = BUILD_VECTOR t164, t166
>> > 48 32 1
>> > 0
>> > Creating new node: t170: v1f32 = BUILD_VECTOR t164
>> > t170: v1f32 = BUILD_VECTOR t164
>> > Assertion failed: (VT.getSizeInBits() == Operand.getValueSizeInBits() &&
>> "Cannot BITCAST between types of different sizes!"), function getNode, file
>> ../lib/CodeGen/SelectionDAG/SelectionDAG.cpp, line 37
>> > 97.
>> >
>> >
>> > Note how what it’s attempting to do here just… doesn’t seem to make much
>> sense, per the printout.
>> >
>> >
>> > —escha
>> >
>> > On Aug 30, 2017, at 3:34 PM, Topper, Craig <craig.topper at intel.com<
>> mailto:craig.topper at intel.com>> wrote:
>> >
>> > Hi Escha,
>> >
>> > This should be calculating the type for the BUILD_VECTOR with same
>> scalar size but less elements. It uses NVT to get the result vector width,
>> and InVT to get the scalar size of the original BUILD_VECTOR. If InVT’s
>> element size is bigger than NVT it gives up.
>> >
>> >       unsigned NumElems = NVT.getSizeInBits() /
>> InVT.getScalarSizeInBits();
>> >       if (NumElems > 0) {
>> >         EVT ExtractVT = EVT::getVectorVT(*DAG.getContext(),
>> >                                          InVT.getVectorElementType(),
>> NumElems);
>> >
>> > Do you have any additional information?
>> >
>> > From: escha at apple.com<mailto:escha at apple.com>
>> > Sent: Wednesday, August 30, 2017 3:05 PM
>> > To: Topper, Craig <craig.topper at intel.com<mailto:craig.topper at intel.com>
>> >
>> > Cc: llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>
>> > Subject: Re: [llvm] r311892 - [DAGCombiner] Teach visitEXTRACT_SUBVECTOR
>> to turn extracts of BUILD_VECTOR into smaller BUILD_VECTORs
>> >
>> > This breaks a test on our backend, but I suspect the patch is slightly
>> bugged. Note the following:
>> >
>> >   // Skip bitcasting
>> >   V = peekThroughBitcast(V);
>> >
>> >   // If the input is a build vector. Try to make a smaller build vector.
>> >   if (V->getOpcode() == ISD::BUILD_VECTOR) {
>> >     if (auto *Idx = dyn_cast<ConstantSDNode>(N->getOperand(1))) {
>> >       EVT InVT = V->getValueType(0);
>> >       unsigned NumElems = NVT.getSizeInBits() /
>> InVT.getScalarSizeInBits();
>> >       if (NumElems > 0) {
>> >         EVT ExtractVT = EVT::getVectorVT(*DAG.getContext(),
>> >                                          InVT.getVectorElementType(),
>> NumElems);
>> >         if (!LegalOperations ||
>> >             TLI.isOperationLegal(ISD::BUILD_VECTOR, ExtractVT)) {
>> >           unsigned IdxVal = Idx->getZExtValue() *
>> NVT.getScalarSizeInBits() /
>> >                             InVT.getScalarSizeInBits();
>> >
>> >           // Extract the pieces from the original build_vector.
>> >           SDValue BuildVec = DAG.getBuildVector(ExtractVT, SDLoc(N),
>> >                                             makeArrayRef(V->op_begin() +
>> IdxVal,
>> >                                                          NumElems));
>> >
>> > Note how we peak through a bitcast, but then use the original type for
>> much of these calculations. So if the element size changed, we break (in
>> this case, a bitcast from v2f32 to v4f16).
>> >
>> > —escha
>> >
>> > On Aug 28, 2017, at 8:28 AM, Craig Topper via llvm-commits <
>> llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>> wrote:
>> >
>> > Author: ctopper
>> > Date: Mon Aug 28 08:28:33 2017
>> > New Revision: 311892
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=311892&view=rev
>> > Log:
>> > [DAGCombiner] Teach visitEXTRACT_SUBVECTOR to turn extracts of
>> BUILD_VECTOR into smaller BUILD_VECTORs
>> >
>> > Only do this before operations are legalized of BUILD_VECTOR is Legal
>> for the target.
>> >
>> > Differential Revision: https://reviews.llvm.org/D37186
>> >
>> > Modified:
>> >    llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> >    llvm/trunk/test/CodeGen/X86/fold-vector-sext-zext.ll
>> >    llvm/trunk/test/CodeGen/X86/widen_extract-1.ll
>> >
>> > Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
>> CodeGen/SelectionDAG/DAGCombiner.cpp?rev=311892&r1=
>> 311891&r2=311892&view=diff
>> > ============================================================
>> ==================
>> >
>> > --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
>> > +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Mon Aug 28
>> 08:28:33 2017
>> > @@ -15157,6 +15157,29 @@ SDValue DAGCombiner::visitEXTRACT_SUBVEC
>> >   // Skip bitcasting
>> >   V = peekThroughBitcast(V);
>> >
>> > +  // If the input is a build vector. Try to make a smaller build vector.
>> > +  if (V->getOpcode() == ISD::BUILD_VECTOR) {
>> > +    if (auto *Idx = dyn_cast<ConstantSDNode>(N->getOperand(1))) {
>> > +      EVT InVT = V->getValueType(0);
>> > +      unsigned NumElems = NVT.getSizeInBits() /
>> InVT.getScalarSizeInBits();
>> > +      if (NumElems > 0) {
>> > +        EVT ExtractVT = EVT::getVectorVT(*DAG.getContext(),
>> > +                                         InVT.getVectorElementType(),
>> NumElems);
>> > +        if (!LegalOperations ||
>> > +            TLI.isOperationLegal(ISD::BUILD_VECTOR, ExtractVT)) {
>> > +          unsigned IdxVal = Idx->getZExtValue() *
>> NVT.getScalarSizeInBits() /
>> > +                            InVT.getScalarSizeInBits();
>> > +
>> > +          // Extract the pieces from the original build_vector.
>> > +          SDValue BuildVec = DAG.getBuildVector(ExtractVT, SDLoc(N),
>> > +                                            makeArrayRef(V->op_begin()
>> + IdxVal,
>> > +                                                         NumElems));
>> > +          return DAG.getBitcast(NVT, BuildVec);
>> > +        }
>> > +      }
>> > +    }
>> > +  }
>> > +
>> >   if (V->getOpcode() == ISD::INSERT_SUBVECTOR) {
>> >     // Handle only simple case where vector being inserted and vector
>> >     // being extracted are of same size.
>> >
>> > Modified: llvm/trunk/test/CodeGen/X86/fold-vector-sext-zext.ll
>> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
>> CodeGen/X86/fold-vector-sext-zext.ll?rev=311892&r1=311891&
>> r2=311892&view=diff
>> > ============================================================
>> ==================
>> > --- llvm/trunk/test/CodeGen/X86/fold-vector-sext-zext.ll (original)
>> > +++ llvm/trunk/test/CodeGen/X86/fold-vector-sext-zext.ll Mon Aug 28
>> 08:28:33 2017
>> > @@ -83,8 +83,7 @@ define <4 x i32> @test_sext_4i8_4i32_und
>> > define <4 x i64> @test_sext_4i8_4i64() {
>> > ; X32-LABEL: test_sext_4i8_4i64:
>> > ; X32:       # BB#0:
>> > -; X32-NEXT:    vmovaps {{.*#+}} xmm0 = [0,0,4294967295,4294967295]
>> > -; X32-NEXT:    vinsertf128 $1, {{\.LCPI.*}}, %ymm0, %ymm0
>> > +; X32-NEXT:    vmovaps {{.*#+}} ymm0 = [0,0,4294967295,4294967295,2,
>> 0,4294967293,4294967295]
>> > ; X32-NEXT:    retl
>> > ;
>> > ; X64-LABEL: test_sext_4i8_4i64:
>> > @@ -102,8 +101,7 @@ define <4 x i64> @test_sext_4i8_4i64() {
>> > define <4 x i64> @test_sext_4i8_4i64_undef() {
>> > ; X32-LABEL: test_sext_4i8_4i64_undef:
>> > ; X32:       # BB#0:
>> > -; X32-NEXT:    vpcmpeqd %xmm0, %xmm0, %xmm0
>> > -; X32-NEXT:    vinsertf128 $1, {{\.LCPI.*}}, %ymm0, %ymm0
>> > +; X32-NEXT:    vmovaps {{.*#+}} ymm0 = <u,u,4294967295,4294967295,u,
>> u,4294967293,4294967295>
>> > ; X32-NEXT:    retl
>> > ;
>> > ; X64-LABEL: test_sext_4i8_4i64_undef:
>> > @@ -245,8 +243,7 @@ define <4 x i32> @test_zext_4i8_4i32() {
>> > define <4 x i64> @test_zext_4i8_4i64() {
>> > ; X32-LABEL: test_zext_4i8_4i64:
>> > ; X32:       # BB#0:
>> > -; X32-NEXT:    vmovaps {{.*#+}} xmm0 = [0,0,255,0]
>> > -; X32-NEXT:    vinsertf128 $1, {{\.LCPI.*}}, %ymm0, %ymm0
>> > +; X32-NEXT:    vmovaps {{.*#+}} ymm0 = [0,0,255,0,2,0,253,0]
>> > ; X32-NEXT:    retl
>> > ;
>> > ; X64-LABEL: test_zext_4i8_4i64:
>> > @@ -300,10 +297,7 @@ define <4 x i32> @test_zext_4i8_4i32_und
>> > define <4 x i64> @test_zext_4i8_4i64_undef() {
>> > ; X32-LABEL: test_zext_4i8_4i64_undef:
>> > ; X32:       # BB#0:
>> > -; X32-NEXT:    vmovaps {{.*#+}} xmm0 = <u,u,255,0>
>> > -; X32-NEXT:    movl $2, %eax
>> > -; X32-NEXT:    vmovd %eax, %xmm1
>> > -; X32-NEXT:    vinsertf128 $1, %xmm1, %ymm0, %ymm0
>> > +; X32-NEXT:    vmovaps {{.*#+}} ymm0 = <u,u,255,0,2,0,u,u>
>> > ; X32-NEXT:    retl
>> > ;
>> > ; X64-LABEL: test_zext_4i8_4i64_undef:
>> >
>> > Modified: llvm/trunk/test/CodeGen/X86/widen_extract-1.ll
>> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
>> CodeGen/X86/widen_extract-1.ll?rev=311892&r1=311891&r2=311892&view=diff
>> > ============================================================
>> ==================
>> > --- llvm/trunk/test/CodeGen/X86/widen_extract-1.ll (original)
>> > +++ llvm/trunk/test/CodeGen/X86/widen_extract-1.ll Mon Aug 28 08:28:33
>> 2017
>> > @@ -7,8 +7,8 @@
>> > define void @convert(<2 x double>* %dst.addr, <3 x double> %src)  {
>> > ; X32-LABEL: convert:
>> > ; X32:       # BB#0: # %entry
>> > -; X32-NEXT:    movups {{[0-9]+}}(%esp), %xmm0
>> > ; X32-NEXT:    movl {{[0-9]+}}(%esp), %eax
>> > +; X32-NEXT:    movups {{[0-9]+}}(%esp), %xmm0
>> > ; X32-NEXT:    movaps %xmm0, (%eax)
>> > ; X32-NEXT:    retl
>> > ;
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>


More information about the llvm-commits mailing list