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