<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 12 (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;}
@font-face
{font-family:Tahoma;
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:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
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 Chandler<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">Is there a protocol (guideline) on code review process? I have done my due diligence on resolving all issues from code reviewers. And I believe I did not
mis-interpret any of their opinions. It will be more constructive and productive if you gave your opinions during the code review process.<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">-Mei<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>
<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""> llvm-commits-bounces@cs.uiuc.edu [mailto:llvm-commits-bounces@cs.uiuc.edu]
<b>On Behalf Of </b>Chandler Carruth<br>
<b>Sent:</b> Friday, July 26, 2013 11:18 PM<br>
<b>To:</b> Matt Arsenault<br>
<b>Cc:</b> llvm-commits@cs.uiuc.edu<br>
<b>Subject:</b> Re: [patch] simplifyCFG for review<o:p></o:p></span></p>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">On Fri, Jul 26, 2013 at 10:36 PM, Matt Arsenault <<a href="mailto:arsenm2@gmail.com" target="_blank">arsenm2@gmail.com</a>> wrote:<o:p></o:p></p>
<div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt">On Jul 26, 2013, at 21:37 , Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>
> This really needs a design discussion not tucked away in a 40-email long patch review thread. Could one of you or Mei start that? In particular, I would like a really compelling reason why this isn't done in a target specific lowering pass, ideally not at
the IR level at all.<o:p></o:p></p>
</div>
<p class="MsoNormal">Wasn't that already started here? <a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-July/063863.html" target="_blank">
http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-July/063863.html</a><o:p></o:p></p>
</blockquote>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">A related discussion was started there, but it is not relevant to this patch AFAICT. I've read the patch now, and I only have more questions and objections to it.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">This patch muddles three totally distinct things, which I'm not happy about being in a single patch:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">1) It adds a CFG transformation that is extremely important on GPUs. This part seems fine, and not really controversial in and of itself. The only real question seemed to be where to put it.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">2) It splits simplifycfg into two passes, one which does target-specific optimizations, and the other which does target-independent canonicalization. This isn't in and of itself bad, but it has done this *only* in the pass. The actual SimplifyCFG
utility remains a single utility. This doesn't seem like the right design as it is now extremely unclear what parts of SimplifyCFG are canonicalization and which parts are optimization. It also piles still more poor design on top of the existing mess of spaghetti
code that is SimplifyCFG (even though it is much better now than it has been historically). It also doesn't factor the *very significant* amount of exsiting target-specific transformations in SimplifyCFG into the optimization partition, or establish good guidelines
for how to layer these two pieces, how to share utility code and to keep clear and separate the different code. All of these should be addressed, and it should be in an independent patch sequence.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">3) It changes one iteration of the simplifycfg pass in the pass manager to be target-aware. This may seem loosely in line with Andy's email, but it really isn't. Andy was hypothesizing about a better pass ordering which I still find interesting.
That said, there are a lot of design questions that would need to be answered to fully implement it (that is, as something other than an experimental thing to get measurements). For example, at what point in that pipeline do we leave the IR? His email didn't
have us leave IR any earlier, but it's not clear whether that will in fact be the right long-term design (it's also not clear that it will be the wrong long term design, i'm ambivalent). But more importantly, his email proposed a substantial new thing that
this doesn't actually provide: a new *pipeline* and factoring lots of different things into it. Instead, this patch changes one run of simplifycfg *in the CGSCC* pass manager to be target aware! That causes the canonicalization pipeline to completely fall
apart.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">#3 is *definitely* wrong. Please revert that part immediately.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I think #2 is also not yet in a state to go into the tree, but since both Evan and Andy seemed OK with it we should at least hear from them before reverting anything.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I would be totally fine with #1 in its own late pass (added by the TargetMachine IMO, not part of the PassManagerBuilder pipeline, much like codegenprep). I'm not sure why that design wasn't considered (i've not read all 40 emails I'm afraid),
so I can't really advocate firmly for or against it.<o:p></o:p></p>
</div>
</div>
</div>
</body>
</html>