<div dir="ltr"><div class="gmail_extra">On Fri, Jul 26, 2013 at 10:36 PM, Matt Arsenault <span dir="ltr"><<a href="mailto:arsenm2@gmail.com" target="_blank" class="cremed">arsenm2@gmail.com</a>></span> wrote:<br><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Jul 26, 2013, at 21:37 , Chandler Carruth <<a href="mailto:chandlerc@google.com" class="cremed">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.<br>

<br>
</div>Wasn't that already started here? <a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-July/063863.html" target="_blank" class="cremed">http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-July/063863.html</a></blockquote>
</div><br></div><div class="gmail_extra">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.</div><div class="gmail_extra">
<br></div><div class="gmail_extra">This patch muddles three totally distinct things, which I'm not happy about being in a single patch:</div><div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">#3 is *definitely* wrong. Please revert that part immediately.</div><div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
</div>