<div dir="ltr">On Wed, May 1, 2013 at 9:53 PM, Nadav Rotem <span dir="ltr"><<a href="mailto:nrotem@apple.com" target="_blank" class="cremed">nrotem@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: nadav<br>
Date: Wed May 1 14:53:30 2013<br>
New Revision: 180875<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=180875&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=180875&view=rev</a><br>
Log:<br>
SROA: Generate selects instead of shuffles when blending values because this is the cannonical form.<br>
Shuffles are more difficult to lower and we usually don't touch them, while we do optimize selects more often.<br></blockquote><div><br></div><div style>Why? (not that i disagree, I just don't think you've explained it fully...)</div>
<div style><br></div><div style>Also, why doesn't instcombine canonicalize shuffles to selects if they are the canonical form? The whole way I picked a form was to look at what would get canonicalized if the user wrote it, and match that. Until the rest of the pipeline is canonicalizing in this way, I worry greatly about this being a deeply fragile hack.</div>
<div style><br></div><div style>Also, as select has *numerous* other problems, it would seem really important to teach the backend to optimize shuffles that are blends rather than paper over this deficiency in SROA.</div>
<div style><br></div><div style>More detailed comments on the patch in line...</div><div style> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/Scalar/SROA.cpp<br>
llvm/trunk/test/Transforms/SROA/vector-promotion.ll<br>
<br>
Modified: llvm/trunk/lib/Transforms/Scalar/SROA.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/SROA.cpp?rev=180875&r1=180874&r2=180875&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/SROA.cpp?rev=180875&r1=180874&r2=180875&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Scalar/SROA.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Scalar/SROA.cpp Wed May 1 14:53:30 2013<br>
@@ -2322,17 +2322,15 @@ static Value *insertVector(IRBuilderTy &<br>
V = IRB.CreateShuffleVector(V, UndefValue::get(V->getType()),<br>
ConstantVector::get(Mask),<br>
Name + ".expand");<br>
- DEBUG(dbgs() << " shuffle1: " << *V << "\n");<br>
+ DEBUG(dbgs() << " shuffle: " << *V << "\n");<br>
<br>
Mask.clear();<br>
for (unsigned i = 0; i != VecTy->getNumElements(); ++i)<br>
- if (i >= BeginIndex && i < EndIndex)<br>
- Mask.push_back(IRB.getInt32(i));<br>
- else<br>
- Mask.push_back(IRB.getInt32(i + VecTy->getNumElements()));<br>
- V = IRB.CreateShuffleVector(V, Old, ConstantVector::get(Mask),<br>
- Name + "insert");<br>
- DEBUG(dbgs() << " shuffle2: " << *V << "\n");<br>
+ Mask.push_back(IRB.getInt1(i >= BeginIndex && i < EndIndex));</blockquote><div><br></div><div style>As this is no longer a shuffle mask, it might be appropriate to use a different variable (and name). At the very least its confusing as this now *must* be a vector of booleans where it previously was not.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+ V = IRB.CreateSelect(ConstantVector::get(Mask), V, Old, Name + "blend");<br>
+<br>
+ DEBUG(dbgs() << " blend: " << *V << "\n");<br></blockquote><div><br></div><div style>Why not call this a select? that's what it is.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
return V;<br>
}<br>
<br>
<br>
Modified: llvm/trunk/test/Transforms/SROA/vector-promotion.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SROA/vector-promotion.ll?rev=180875&r1=180874&r2=180875&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/SROA/vector-promotion.ll?rev=180875&r1=180874&r2=180875&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/SROA/vector-promotion.ll (original)<br>
+++ llvm/trunk/test/Transforms/SROA/vector-promotion.ll Wed May 1 14:53:30 2013<br>
@@ -224,26 +224,26 @@ entry:<br>
%a.cast0 = bitcast i32* %a.gep0 to <2 x i32>*<br>
store <2 x i32> <i32 0, i32 0>, <2 x i32>* %a.cast0<br>
; CHECK-NOT: store<br>
-; CHECK: %[[insert1:.*]] = shufflevector <4 x i32> <i32 0, i32 0, i32 undef, i32 undef>, <4 x i32> undef, <4 x i32> <i32 0, i32 1, {{.*}}><br>
+; CHECK: select <4 x i1> <i1 true, i1 true, i1 false, i1 false><br></blockquote><div><br></div><div style>Here and below you have significantly decreased the specificity of the check. Can you please preserve that? I really dislike checking instructions occur but not checking that the values they produce are used in the correct context. While sometimes it is impossible, here the code *already did that*. =/</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
%a.gep1 = getelementptr <4 x i32>* %a, i32 0, i32 1<br>
%a.cast1 = bitcast i32* %a.gep1 to <2 x i32>*<br>
store <2 x i32> <i32 1, i32 1>, <2 x i32>* %a.cast1<br>
-; CHECK-NEXT: %[[insert2:.*]] = shufflevector <4 x i32> <i32 undef, i32 1, i32 1, i32 undef>, <4 x i32> %[[insert1]], <4 x i32> <i32 4, i32 1, i32 2, {{.*}}><br>
+; CHECK-NEXT: select <4 x i1> <i1 false, i1 true, i1 true, i1 false><br>
<br>
%a.gep2 = getelementptr <4 x i32>* %a, i32 0, i32 2<br>
%a.cast2 = bitcast i32* %a.gep2 to <2 x i32>*<br>
store <2 x i32> <i32 2, i32 2>, <2 x i32>* %a.cast2<br>
-; CHECK-NEXT: %[[insert3:.*]] = shufflevector <4 x i32> <i32 undef, i32 undef, i32 2, i32 2>, <4 x i32> %[[insert2]], <4 x i32> <i32 4, i32 5, i32 2, i32 3><br>
+; CHECK-NEXT: select <4 x i1> <i1 false, i1 false, i1 true, i1 true><br>
<br>
%a.gep3 = getelementptr <4 x i32>* %a, i32 0, i32 3<br>
store i32 3, i32* %a.gep3<br>
-; CHECK-NEXT: %[[insert4:.*]] = insertelement <4 x i32> %[[insert3]], i32 3, i32 3<br>
+; CHECK-NEXT: insertelement <4 x i32><br>
<br>
%ret = load <4 x i32>* %a<br>
<br>
ret <4 x i32> %ret<br>
-; CHECK-NEXT: ret <4 x i32> %[[insert4]]<br>
+; CHECK-NEXT: ret <4 x i32><br>
}<br>
<br>
define <4 x i32> @test_subvec_load() {<br>
@@ -291,27 +291,27 @@ entry:<br>
%a.cast0 = bitcast float* %a.gep0 to i8*<br>
call void @llvm.memset.p0i8.i32(i8* %a.cast0, i8 0, i32 8, i32 0, i1 false)<br>
; CHECK-NOT: store<br>
-; CHECK: %[[insert1:.*]] = shufflevector <4 x float> <float 0.000000e+00, float 0.000000e+00, float undef, float undef>, <4 x float> undef, <4 x i32> <i32 0, i32 1, {{.*}}><br>
+; CHECK: select <4 x i1> <i1 true, i1 true, i1 false, i1 false><br>
<br>
%a.gep1 = getelementptr <4 x float>* %a, i32 0, i32 1<br>
%a.cast1 = bitcast float* %a.gep1 to i8*<br>
call void @llvm.memset.p0i8.i32(i8* %a.cast1, i8 1, i32 8, i32 0, i1 false)<br>
-; CHECK-NEXT: %[[insert2:.*]] = shufflevector <4 x float> <float undef, float 0x3820202020000000, float 0x3820202020000000, float undef>, <4 x float> %[[insert1]], <4 x i32> <i32 4, i32 1, i32 2, {{.*}}><br>
+; CHECK-NEXT: select <4 x i1> <i1 false, i1 true, i1 true, i1 false><br>
<br>
%a.gep2 = getelementptr <4 x float>* %a, i32 0, i32 2<br>
%a.cast2 = bitcast float* %a.gep2 to i8*<br>
call void @llvm.memset.p0i8.i32(i8* %a.cast2, i8 3, i32 8, i32 0, i1 false)<br>
-; CHECK-NEXT: %[[insert3:.*]] = shufflevector <4 x float> <float undef, float undef, float 0x3860606060000000, float 0x3860606060000000>, <4 x float> %[[insert2]], <4 x i32> <i32 4, i32 5, i32 2, i32 3><br>
+; CHECK-NEXT: select <4 x i1> <i1 false, i1 false, i1 true, i1 true><br>
<br>
%a.gep3 = getelementptr <4 x float>* %a, i32 0, i32 3<br>
%a.cast3 = bitcast float* %a.gep3 to i8*<br>
call void @llvm.memset.p0i8.i32(i8* %a.cast3, i8 7, i32 4, i32 0, i1 false)<br>
-; CHECK-NEXT: %[[insert4:.*]] = insertelement <4 x float> %[[insert3]], float 0x38E0E0E0E0000000, i32 3<br>
+; CHECK-NEXT: insertelement <4 x float><br>
<br>
%ret = load <4 x float>* %a<br>
<br>
ret <4 x float> %ret<br>
-; CHECK-NEXT: ret <4 x float> %[[insert4]]<br>
+; CHECK-NEXT: ret <4 x float><br>
}<br>
<br>
define <4 x float> @test_subvec_memcpy(i8* %x, i8* %y, i8* %z, i8* %f, i8* %out) {<br>
@@ -326,7 +326,7 @@ entry:<br>
; CHECK: %[[xptr:.*]] = bitcast i8* %x to <2 x float>*<br>
; CHECK-NEXT: %[[x:.*]] = load <2 x float>* %[[xptr]]<br>
; CHECK-NEXT: %[[expand_x:.*]] = shufflevector <2 x float> %[[x]], <2 x float> undef, <4 x i32> <i32 0, i32 1, i32 undef, i32 undef><br>
-; CHECK-NEXT: %[[insert_x:.*]] = shufflevector <4 x float> %[[expand_x]], <4 x float> undef, <4 x i32> <i32 0, i32 1, {{.*}}><br>
+; CHECK-NEXT: select <4 x i1> <i1 true, i1 true, i1 false, i1 false><br>
<br>
%a.gep1 = getelementptr <4 x float>* %a, i32 0, i32 1<br>
%a.cast1 = bitcast float* %a.gep1 to i8*<br>
@@ -334,7 +334,7 @@ entry:<br>
; CHECK-NEXT: %[[yptr:.*]] = bitcast i8* %y to <2 x float>*<br>
; CHECK-NEXT: %[[y:.*]] = load <2 x float>* %[[yptr]]<br>
; CHECK-NEXT: %[[expand_y:.*]] = shufflevector <2 x float> %[[y]], <2 x float> undef, <4 x i32> <i32 undef, i32 0, i32 1, i32 undef><br>
-; CHECK-NEXT: %[[insert_y:.*]] = shufflevector <4 x float> %[[expand_y]], <4 x float> %[[insert_x]], <4 x i32> <i32 4, i32 1, i32 2, {{.*}}><br>
+; CHECK-NEXT: select <4 x i1> <i1 false, i1 true, i1 true, i1 false><br>
<br>
%a.gep2 = getelementptr <4 x float>* %a, i32 0, i32 2<br>
%a.cast2 = bitcast float* %a.gep2 to i8*<br>
@@ -342,14 +342,14 @@ entry:<br>
; CHECK-NEXT: %[[zptr:.*]] = bitcast i8* %z to <2 x float>*<br>
; CHECK-NEXT: %[[z:.*]] = load <2 x float>* %[[zptr]]<br>
; CHECK-NEXT: %[[expand_z:.*]] = shufflevector <2 x float> %[[z]], <2 x float> undef, <4 x i32> <i32 undef, i32 undef, i32 0, i32 1><br>
-; CHECK-NEXT: %[[insert_z:.*]] = shufflevector <4 x float> %[[expand_z]], <4 x float> %[[insert_y]], <4 x i32> <i32 4, i32 5, i32 2, i32 3><br>
+; CHECK-NEXT: select <4 x i1> <i1 false, i1 false, i1 true, i1 true><br>
<br>
%a.gep3 = getelementptr <4 x float>* %a, i32 0, i32 3<br>
%a.cast3 = bitcast float* %a.gep3 to i8*<br>
call void @llvm.memcpy.p0i8.p0i8.i32(i8* %a.cast3, i8* %f, i32 4, i32 0, i1 false)<br>
; CHECK-NEXT: %[[fptr:.*]] = bitcast i8* %f to float*<br>
; CHECK-NEXT: %[[f:.*]] = load float* %[[fptr]]<br>
-; CHECK-NEXT: %[[insert_f:.*]] = insertelement <4 x float> %[[insert_z]], float %[[f]], i32 3<br>
+; CHECK-NEXT: %[[insert_f:.*]] = insertelement <4 x float><br>
<br>
call void @llvm.memcpy.p0i8.p0i8.i32(i8* %out, i8* %a.cast2, i32 8, i32 0, i1 false)<br>
; CHECK-NEXT: %[[outptr:.*]] = bitcast i8* %out to <2 x float>*<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" class="cremed">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div>