<div dir="ltr"><div><div><div>Hi Zvi,<br><br></div>thanks for this patch.<br></div><br>I noticed that method visitShuffleVectorInst() knows already how to simplify shuffles (using `SimplifyDemandedVectorElts()`) and canonicalize shuffles according to the following rules:<br><br> // Canonicalize shuffle(x ,x,mask) -> shuffle(x, undef,mask')<br> // Canonicalize shuffle(undef,x,mask) -> shuffle(x, undef,mask').<br><br>SimplifyDemandedVectorElts() would be able to identify shuffles where only one operands is used. That means, SimplifyDemandedVectorElts() is able to propagate Undef to unused operands of a shuffle.<br><br></div><div>I think that the logic in SimplifyShuffleVectorInst() could be much simpler if only we could assume that shuffles have been already canonicalized and simplified using `SimplifyDemandedVectorElts()`.<br>If we assume canonical shuffles in input, then the following scenario can never happen:<br><br> // If only one of the operands is constant, constant fold the shuffle if the<br> // mask does not select elements from the variable operand.<br><br></div><div>That is because 1) SimplifyDemandedVectorElts() would have replaced the unused operand with Undef, and 2) the Undef would always be the second operand.<br><br></div><div>The question is: should we consider moving all the shuffle simplification logic from InstCombineVectorOps.cpp to InstructionSimplify.cpp?<br><br>I am under the impression that all that logic should live in InstructionSimplify.cpp.<br></div><div>It would also help simplifying the logic in SimplifyShuffleVectorInst() (we would only need the first rule of that function which knows how to fold a shuffle with operands that are both constant vectors/undef).<br><br></div><div>Not sure if this all makes sense, but I hope it helps improving that code.<br></div><div><br></div><div>-Andrea<br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Apr 4, 2017 at 5:47 AM, Zvi Rackover 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">Author: zvi<br>
Date: Mon Apr 3 23:47:57 2017<br>
New Revision: 299412<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=299412&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=299412&view=rev</a><br>
Log:<br>
InstCombine: Use the InstSimplify hook for shufflevector<br>
<br>
Summary: Start using the recently added InstSimplify hook for shuffles in the respective InstCombine visitor.<br>
<br>
Reviewers: spatel, RKSimon, craig.topper, majnemer<br>
<br>
Reviewed By: majnemer<br>
<br>
Subscribers: majnemer, llvm-commits<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D31526" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D31526</a><br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/<wbr>InstCombine/<wbr>InstCombineVectorOps.cpp<br>
<br>
Modified: llvm/trunk/lib/Transforms/<wbr>InstCombine/<wbr>InstCombineVectorOps.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineVectorOps.cpp?rev=299412&r1=299411&r2=299412&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Transforms/InstCombine/<wbr>InstCombineVectorOps.cpp?rev=<wbr>299412&r1=299411&r2=299412&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/lib/Transforms/<wbr>InstCombine/<wbr>InstCombineVectorOps.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/<wbr>InstCombine/<wbr>InstCombineVectorOps.cpp Mon Apr 3 23:47:57 2017<br>
@@ -1140,12 +1140,11 @@ Instruction *InstCombiner::visitShuffleV<br>
SmallVector<int, 16> Mask = SVI.getShuffleMask();<br>
Type *Int32Ty = Type::getInt32Ty(SVI.<wbr>getContext());<br>
<br>
- bool MadeChange = false;<br>
-<br>
- // Undefined shuffle mask -> undefined value.<br>
- if (isa<UndefValue>(SVI.<wbr>getOperand(2)))<br>
- return replaceInstUsesWith(SVI, UndefValue::get(SVI.getType())<wbr>);<br>
+ if (auto *V = SimplifyShuffleVectorInst(LHS, RHS, SVI.getMask(),<br>
+ SVI.getType(), DL, &TLI, &DT, &AC))<br>
+ return replaceInstUsesWith(SVI, V);<br>
<br>
+ bool MadeChange = false;<br>
unsigned VWidth = SVI.getType()-><wbr>getVectorNumElements();<br>
<br>
APInt UndefElts(VWidth, 0);<br>
<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>
</blockquote></div><br></div>