<div dir="ltr"><div>Hi Jay -</div><div><br></div><div>Yes, this change carried some risk of regressions, and we should still be able to optimize this case. We may need to add or loosen a fold for splat-of-insert:</div><div>define <4 x float> @f(<4 x float> %x) {<br> %ins3 = insertelement <4 x float> %x, float 3.000000e+00, i32 3<br> %splat0 = shufflevector <4 x float> %ins3, <4 x float> undef, <4 x i32> zeroinitializer<br> %splat3 = shufflevector <4 x float> %ins3, <4 x float> undef, <4 x i32> <i32 3, i32 3, i32 3, i32 3><br> %r = fadd <4 x float> %splat0, %splat3<br> ret <4 x float> %r<br>}</div><div><br></div><div>SimplifyDemandedVectorElts() may not get this because %ins3 has multiple uses, but I didn't step through that yet to confirm. I can work on this if you haven't started already. Let me know. </div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Dec 9, 2019 at 10:36 AM Jay Foad <<a href="mailto:jay.foad@gmail.com">jay.foad@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Sanjay,<br>
<br>
I'm looking at some missed optimizations caused by D70246. Here's a test case:<br>
<br>
define <4 x float> @f(i32 %t32, <4 x float>* %t24) {<br>
.entry:<br>
%t43 = insertelement <3 x i32> undef, i32 %t32, i32 2<br>
%t44 = bitcast <3 x i32> %t43 to <3 x float><br>
%t45 = shufflevector <3 x float> %t44, <3 x float> undef, <4 x i32><br>
<i32 0, i32 undef, i32 undef, i32 undef><br>
%t46 = shufflevector <3 x float> %t44, <3 x float> undef, <4 x i32><br>
<i32 undef, i32 1, i32 undef, i32 undef><br>
%t47 = shufflevector <3 x float> %t44, <3 x float> undef, <4 x i32><br>
<i32 undef, i32 undef, i32 2, i32 undef><br>
%t48 = insertelement <4 x float> %t45, float 1.000000e+00, i32 3<br>
%t49 = shufflevector <4 x float> %t48, <4 x float> %t46, <4 x i32><br>
<i32 0, i32 5, i32 undef, i32 3><br>
%t50 = shufflevector <4 x float> %t49, <4 x float> %t47, <4 x i32><br>
<i32 0, i32 1, i32 6, i32 3><br>
%t55 = load <4 x float>, <4 x float>* %t24, align 16<br>
%t58 = getelementptr <4 x float>, <4 x float>* %t24, i64 1<br>
%t59 = load <4 x float>, <4 x float>* %t58, align 16<br>
%t62 = getelementptr <4 x float>, <4 x float>* %t24, i64 2<br>
%t63 = load <4 x float>, <4 x float>* %t62, align 16<br>
%t66 = getelementptr <4 x float>, <4 x float>* %t24, i64 3<br>
%t67 = load <4 x float>, <4 x float>* %t66, align 16<br>
%t69 = shufflevector <4 x float> %t50, <4 x float> undef, <4 x i32><br>
zeroinitializer<br>
%t71 = fmul <4 x float> %t55, %t69<br>
%t72 = shufflevector <3 x float> %t44, <3 x float> undef, <4 x i32><br>
<i32 1, i32 1, i32 1, i32 1><br>
%t74 = fmul <4 x float> %t59, %t72<br>
%t75 = fadd <4 x float> %t71, %t74<br>
%t76 = shufflevector <3 x float> %t44, <3 x float> undef, <4 x i32><br>
<i32 2, i32 2, i32 2, i32 2><br>
%t78 = fmul <4 x float> %t63, %t76<br>
%t79 = fadd <4 x float> %t75, %t78<br>
%t80 = shufflevector <4 x float> %t50, <4 x float> undef, <4 x i32><br>
<i32 3, i32 3, i32 3, i32 3><br>
%t82 = fmul <4 x float> %t67, %t80<br>
%t83 = fadd <4 x float> %t79, %t82<br>
ret <4 x float> %t83<br>
}<br>
<br>
Before D70246, opt -instcombine gives this:<br>
<br>
define <4 x float> @f(i32 %t32, <4 x float>* %t24) {<br>
.entry:<br>
%t43 = insertelement <3 x i32> undef, i32 %t32, i32 2<br>
%t44 = bitcast <3 x i32> %t43 to <3 x float><br>
%t55 = load <4 x float>, <4 x float>* %t24, align 16<br>
%t58 = getelementptr <4 x float>, <4 x float>* %t24, i64 1<br>
%t59 = load <4 x float>, <4 x float>* %t58, align 16<br>
%t62 = getelementptr <4 x float>, <4 x float>* %t24, i64 2<br>
%t63 = load <4 x float>, <4 x float>* %t62, align 16<br>
%t66 = getelementptr <4 x float>, <4 x float>* %t24, i64 3<br>
%t67 = load <4 x float>, <4 x float>* %t66, align 16<br>
%t69 = shufflevector <3 x float> %t44, <3 x float> undef, <4 x i32><br>
zeroinitializer<br>
%t71 = fmul <4 x float> %t55, %t69<br>
%t72 = shufflevector <3 x float> %t44, <3 x float> undef, <4 x i32><br>
<i32 1, i32 1, i32 1, i32 1><br>
%t74 = fmul <4 x float> %t59, %t72<br>
%t75 = fadd <4 x float> %t71, %t74<br>
%t76 = shufflevector <3 x float> %t44, <3 x float> undef, <4 x i32><br>
<i32 2, i32 2, i32 2, i32 2><br>
%t78 = fmul <4 x float> %t63, %t76<br>
%t79 = fadd <4 x float> %t75, %t78<br>
%t83 = fadd <4 x float> %t79, %t67<br>
ret <4 x float> %t83<br>
}<br>
<br>
After D70246, opt -instcombine gives this:<br>
<br>
define <4 x float> @f(i32 %t32, <4 x float>* %t24) {<br>
.entry:<br>
%t43 = insertelement <3 x i32> undef, i32 %t32, i32 2<br>
%t44 = bitcast <3 x i32> %t43 to <3 x float><br>
%t45 = shufflevector <3 x float> %t44, <3 x float> undef, <4 x i32><br>
<i32 0, i32 undef, i32 undef, i32 undef><br>
%t48 = insertelement <4 x float> %t45, float 1.000000e+00, i32 3<br>
%t55 = load <4 x float>, <4 x float>* %t24, align 16<br>
%t58 = getelementptr <4 x float>, <4 x float>* %t24, i64 1<br>
%t59 = load <4 x float>, <4 x float>* %t58, align 16<br>
%t62 = getelementptr <4 x float>, <4 x float>* %t24, i64 2<br>
%t63 = load <4 x float>, <4 x float>* %t62, align 16<br>
%t66 = getelementptr <4 x float>, <4 x float>* %t24, i64 3<br>
%t67 = load <4 x float>, <4 x float>* %t66, align 16<br>
%t69 = shufflevector <4 x float> %t48, <4 x float> undef, <4 x i32><br>
zeroinitializer<br>
%t71 = fmul <4 x float> %t55, %t69<br>
%t72 = shufflevector <3 x float> %t44, <3 x float> undef, <4 x i32><br>
<i32 1, i32 1, i32 1, i32 1><br>
%t74 = fmul <4 x float> %t59, %t72<br>
%t75 = fadd <4 x float> %t71, %t74<br>
%t76 = shufflevector <3 x float> %t44, <3 x float> undef, <4 x i32><br>
<i32 2, i32 2, i32 2, i32 2><br>
%t78 = fmul <4 x float> %t63, %t76<br>
%t79 = fadd <4 x float> %t75, %t78<br>
%t80 = shufflevector <4 x float> %t48, <4 x float> undef, <4 x i32><br>
<i32 3, i32 3, i32 3, i32 3><br>
%t82 = fmul <4 x float> %t67, %t80<br>
%t83 = fadd <4 x float> %t79, %t82<br>
ret <4 x float> %t83<br>
}<br>
<br>
Notice that it has failed to simplify %t80, which extracts element 3<br>
from %t48, which is obviously just the constant 1.0.<br>
<br>
Would you expect the change you committed to have this kind of effect?<br>
Do you agree that simplifying %t80 is still valid, even with the new<br>
interpretation of the rules about shufflevector and undef and poison?<br>
<br>
Thanks,<br>
Jay.<br>
<br>
On Thu, 14 Nov 2019 at 16:25, Sanjay Patel via Phabricator via<br>
llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
><br>
> spatel created this revision.<br>
> spatel added reviewers: efriedma, regehr, aqjune, nlopes, RKSimon, lebedev.ri, liuz.<br>
> Herald added subscribers: hiraditya, mcrosier.<br>
> Herald added a project: LLVM.<br>
><br>
> Given a shuffle that includes undef elements in an otherwise identity mask like:<br>
><br>
> define <4 x float> @shuffle(<4 x float> %arg) {<br>
> %shuf = shufflevector <4 x float> %arg, <4 x float> undef, <4 x i32> <i32 undef, i32 1, i32 2, i32 3><br>
> ret <4 x float> %shuf<br>
> }<br>
><br>
> We were simplifying that to the input operand.<br>
><br>
> But as discussed in PR43958:<br>
> <a href="https://bugs.llvm.org/show_bug.cgi?id=43958" rel="noreferrer" target="_blank">https://bugs.llvm.org/show_bug.cgi?id=43958</a><br>
> ...that means that per-vector-element poison that would be stopped by the shuffle can now leak to the result.<br>
><br>
> For reference, this transform was added long ago with:<br>
> rL142671 <<a href="https://reviews.llvm.org/rL142671" rel="noreferrer" target="_blank">https://reviews.llvm.org/rL142671</a>><br>
><br>
> Also note that we still have (and there are tests for) the same transform with no undef elements in the mask (a fully-defined identity mask). I don't think there's any controversy about that case - it's a valid transform under any interpretation of shufflevector/undef/poison.<br>
><br>
> Looking at a few of the diffs into codegen, I don't see any difference in final asm. So depending on your perspective, that's good (no real loss of optimization power) or bad (poison exists in the DAG, so we only partially fixed the bug).<br>
><br>
><br>
> <a href="https://reviews.llvm.org/D70246" rel="noreferrer" target="_blank">https://reviews.llvm.org/D70246</a><br>
><br>
> Files:<br>
> llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp<br>
> llvm/test/Transforms/InstCombine/X86/clmulqdq.ll<br>
> llvm/test/Transforms/InstCombine/X86/x86-avx2.ll<br>
> llvm/test/Transforms/InstCombine/X86/x86-avx512.ll<br>
> llvm/test/Transforms/InstCombine/X86/x86-f16c.ll<br>
> llvm/test/Transforms/InstCombine/X86/x86-pack.ll<br>
> llvm/test/Transforms/InstCombine/X86/x86-pshufb.ll<br>
> llvm/test/Transforms/InstCombine/X86/x86-sse.ll<br>
> llvm/test/Transforms/InstCombine/X86/x86-sse41.ll<br>
> llvm/test/Transforms/InstCombine/X86/x86-sse4a.ll<br>
> llvm/test/Transforms/InstCombine/X86/x86-vector-shifts.ll<br>
> llvm/test/Transforms/InstCombine/X86/x86-vpermil.ll<br>
> llvm/test/Transforms/InstCombine/X86/x86-xop.ll<br>
> llvm/test/Transforms/InstCombine/shuffle_select.ll<br>
> llvm/test/Transforms/InstCombine/vec_demanded_elts.ll<br>
> llvm/test/Transforms/InstCombine/vec_shuffle.ll<br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>