[llvm] r272505 - [PM] Port ReversePostOrderFunctionAttrs to the new PM

[Daniel Berlin via llvm-commits]

On Thu, Jun 16, 2016 at 1:49 PM, Davide Italiano <dccitaliano at gmail.com>
wrote:

> On Thu, Jun 16, 2016 at 9:48 AM, Daniel Berlin <dberlin at dberlin.org>
> wrote:
> > Hey guys:
> >
> > I'm a little concerned how some of these passes are being converted.
> >
> > For example, mergedloadstoremotion was entirely turned into a bunch of
> > static helpers that pass around lots of what was previously simple class
> > state.
> >
> > With no review, or discussion.
> >
> > I find it *much* harder to understand, and in fact, with the memoryssa
> > version coming, it would mean tons of functions now get passed 4 or 5
> random
> > things like AA, DT, MD, MSSA, etc.
> >
> > This doesn't make a lot of sense, and it could have been easily avoided
> > entirely. The simplest refactoring would have been to modify the existing
> > class a bit so it looks closer to the classes we use for utils, pass the
> > right pieces in the constructor, and leave the rest alone.
> >
> > In fact, i've asked for that particular set of patches to be reverted.
> >
> > I'm not opposed to simply committing them when they convert one thing to
> a
> > static helper or whatever.
> > But i think if what is happening is wholesale conversion of classes to
> > statics, etc, either it needs review, or we shouldn't be doing it that
> way.
> >
> > Because, IMHO, in at least that case, it ended up *much* worse than the
> > original pass.
> >
>
> I missed this thread (only saw the comment on the original patch).
> The pass was reverted. Again, I recommend you to take a look at the
> existing passes ported (at least by me) and let me know if there's
> something you don't like or whatever so that I can try to fix/revert.
>


I haven't followed every single one. I think the way memoryssa, gvn, and
other things i stared at looked fine. I'll take a further look.

I think the *actual conversion* part (IE creating the legacy wrapper class
and new pass manager class) sounds fine and looked exactly right.
It was the refactoring done before that conversion that seemed really odd.
I just don't think you need to or should refactor the entire file's
logicinto static functions to make it usable from the two new classes.
In this case, it's a 50 line patch to make the existing MLSM class have a
constructor, remove getanalysisusage, etc, and then just create MLSM
objects in the legacy and new passmanager classes/etc to run it (MemorySSA
and GVN are done this way, for example)  In this case it will then function
identically with no other changes.

If we want to refactor it after that, we can certainly have that discussion
(i'd be pretty strongly opposed to converting it to a non-class, but of
course, if the consensus is against me, i'll accept it), but i think it
wasn't necessary in this case, and we should generally take the least
invasive way to convert, and if it *needs*  to be invasive, have a real
review for it.

>
> --
> Davide
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160616/528c7d1e/attachment.html>