<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=us-ascii">
<meta name="Generator" content="Microsoft Word 14 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
@font-face
{font-family:Verdana;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
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:"Verdana","sans-serif";
color:purple;
font-weight:normal;
font-style:normal;
text-decoration:none none;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.25in 1.0in 1.25in;}
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" style="margin-left:.5in">In the current transformation the old vector cast goes away because its only user is removed. In the new transformation that you are proposing you are adding a new cast instruction, but the old vector cast still
stays in place. So, after your transformation we will have one more cast operation.<o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:purple"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:purple">The removed check is that the extract has only one use. The check that the cast has only one use remains and after its only use, the extract, is removed, it’s
eliminated as well.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:purple"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:purple">Fixed patch attached.
<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:purple"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:purple">Regards, Anat<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:purple"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:purple"><o:p> </o:p></span></p>
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Nadav Rotem [mailto:nrotem@apple.com]
<br>
<b>Sent:</b> Wednesday, April 17, 2013 18:58<br>
<b>To:</b> Shemer, Anat<br>
<b>Cc:</b> llvm-commits@cs.uiuc.edu<br>
<b>Subject:</b> Re: [PATCH] InstCombine: scalarize vector phis<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">I’m not sure that I follow. The proposed transformation does the same, it creates only one cast. You can see this in the provided test.<o:p></o:p></p>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">In the current transformation the old vector cast goes away because its only user is removed. In the new transformation that you are proposing you are adding a new cast instruction, but the old vector cast still stays in place. So, after
your transformation we will have one more cast operation.<o:p></o:p></p>
</div>
<p class="MsoNormal"><br>
<br>
<o:p></o:p></p>
<div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">>> I did not read the code but from a quick peek I noticed that the docs are not capitalized and punctuated properly and the variable names are funny, and you left some code commented out and there are extra braces, etc. <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:purple"> </span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:purple">I made some corrections.</span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:purple"> </span><o:p></o:p></p>
</div>
</div>
</div>
<div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">+Instruction *InstCombiner::scalarizePHI(ExtractElementInst &EI, PHINode *PN) {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ // Verify that it has only 2 uses. If so, it's known at this point that one<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ // operand is PHI and the other is an extractelement node.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ if (PN->hasNUses(2)) {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ // Find the PHI user that is not the extractelement node.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Please use early-exits. There is no need to indent all of the code in the function if we can simply return null. <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">+ Value::use_iterator iu = PN->use_begin();<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ Instruction *PHIUser = dyn_cast<Instruction>(*iu);<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ if (PHIUser == cast<Instruction>(&EI)) {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ PHIUser = cast<Instruction>(*(++iu));<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ }<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">No need for braces around the PHIUser … Also, can you add comment that explains this code ?<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">+ // Verify that this PHI user has one use, which is the PHI itself,<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ // and that it is a binary operation which is cheap to scalarize.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ if (PHIUser->hasOneUse() && PHIUser->use_back() == PN &&<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ (isa<BinaryOperator>(PHIUser)) &&<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">+ CheapToScalarize(PHIUser, true)) {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Early exit. <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Except for these comments it looks good to me. <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</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>