<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman",serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
span.EmailStyle17
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Hi Andrea,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Your suggestion makes a lot of sense, and I too had similar thoughts about the benefits of moving logic from InstCombine to InstructionSimplify, and the benefits
of canonicalizing early. ‘foldOrCommuteConstant’ is an example of how early canonicalization is done on binary operations to allow assumption to be held – we should definitely have a mechanism for shuffles.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">I was planning on first adding a few shuffle simplifications which will show how badly we need to canonicalize early to avoid duplications and redundancies, and
follow with some canonicalization and refactoring as NFC changes. Then start another iteration of adding more cases that can be handled by SimplifyShuffleVectorInst, followed by more cleanup, and so on.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Would appreciate feedback and comments.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Thanks, Zvi<o:p></o:p></span></p>
<p class="MsoNormal"><a name="_MailEndCompose"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></a></p>
<p class="MsoNormal"><a name="_____replyseparator"></a><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> Andrea Di Biagio [mailto:andrea.dibiagio@gmail.com]
<br>
<b>Sent:</b> Tuesday, April 04, 2017 17:11<br>
<b>To:</b> Rackover, Zvi <zvi.rackover@intel.com><br>
<b>Cc:</b> llvm-commits <llvm-commits@lists.llvm.org><br>
<b>Subject:</b> Re: [llvm] r299412 - InstCombine: Use the InstSimplify hook for shufflevector<o:p></o:p></span></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt">Hi Zvi,<o:p></o:p></p>
</div>
<p class="MsoNormal">thanks for this patch.<o:p></o:p></p>
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><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.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt">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.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt">That is because 1) SimplifyDemandedVectorElts() would have replaced the unused operand with Undef, and 2) the Undef would always be the second operand.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">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.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt">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).<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">Not sure if this all makes sense, but I hope it helps improving that code.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">-Andrea<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">On Tue, Apr 4, 2017 at 5:47 AM, Zvi Rackover via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<o:p></o:p></p>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm">
<p class="MsoNormal">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" target="_blank">
http://llvm.org/viewvc/llvm-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" target="_blank">
https://reviews.llvm.org/D31526</a><br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/InstCombine/InstCombineVectorOps.cpp<br>
<br>
Modified: llvm/trunk/lib/Transforms/InstCombine/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" target="_blank">
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineVectorOps.cpp?rev=299412&r1=299411&r2=299412&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/InstCombine/InstCombineVectorOps.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/InstCombine/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.getContext());<br>
<br>
- bool MadeChange = false;<br>
-<br>
- // Undefined shuffle mask -> undefined value.<br>
- if (isa<UndefValue>(SVI.getOperand(2)))<br>
- return replaceInstUsesWith(SVI, UndefValue::get(SVI.getType()));<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()->getVectorNumElements();<br>
<br>
APInt UndefElts(VWidth, 0);<br>
<br>
<br>
_______________________________________________<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" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><o:p></o:p></p>
</blockquote>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
<p>---------------------------------------------------------------------<br>
Intel Israel (74) Limited</p>
<p>This e-mail and any attachments may contain confidential material for<br>
the sole use of the intended recipient(s). Any review or distribution<br>
by others is strictly prohibited. If you are not the intended<br>
recipient, please contact the sender and delete all copies.</p></body>
</html>