[patch] simplifyCFG for review

[Chandler Carruth]

On Fri, Jul 26, 2013 at 10:36 PM, Matt Arsenault <arsenm2 at gmail.com> wrote:

> On Jul 26, 2013, at 21:37 , Chandler Carruth <chandlerc at google.com> wrote:
> > 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.
>
> Wasn't that already started here?
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-July/063863.html


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.

This patch muddles three totally distinct things, which I'm not happy about
being in a single patch:

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.

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.

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.

#3 is *definitely* wrong. Please revert that part immediately.

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.

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130726/34768847/attachment.html>