<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 16, 2016 at 1:49 PM, Davide Italiano <span dir="ltr"><<a href="mailto:dccitaliano@gmail.com" target="_blank">dccitaliano@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Thu, Jun 16, 2016 at 9:48 AM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>> wrote:<br>
> Hey guys:<br>
><br>
> I'm a little concerned how some of these passes are being converted.<br>
><br>
> For example, mergedloadstoremotion was entirely turned into a bunch of<br>
> static helpers that pass around lots of what was previously simple class<br>
> state.<br>
><br>
> With no review, or discussion.<br>
><br>
> I find it *much* harder to understand, and in fact, with the memoryssa<br>
> version coming, it would mean tons of functions now get passed 4 or 5 random<br>
> things like AA, DT, MD, MSSA, etc.<br>
><br>
> This doesn't make a lot of sense, and it could have been easily avoided<br>
> entirely. The simplest refactoring would have been to modify the existing<br>
> class a bit so it looks closer to the classes we use for utils, pass the<br>
> right pieces in the constructor, and leave the rest alone.<br>
><br>
> In fact, i've asked for that particular set of patches to be reverted.<br>
><br>
> I'm not opposed to simply committing them when they convert one thing to a<br>
> static helper or whatever.<br>
> But i think if what is happening is wholesale conversion of classes to<br>
> statics, etc, either it needs review, or we shouldn't be doing it that way.<br>
><br>
> Because, IMHO, in at least that case, it ended up *much* worse than the<br>
> original pass.<br>
><br>
<br>
</span>I missed this thread (only saw the comment on the original patch).<br>
The pass was reverted. Again, I recommend you to take a look at the<br>
existing passes ported (at least by me) and let me know if there's<br>
something you don't like or whatever so that I can try to fix/revert.<br></blockquote><div><br></div><div><br>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.</div><div><br></div><div>I think the *actual conversion* part (IE creating the legacy wrapper class and new pass manager class) sounds fine and looked exactly right.</div><div>It was the refactoring done before that conversion that seemed really odd.</div><div>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.<br></div><div>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.</div><div><br></div><div>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.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
--<br>
Davide<br>
</blockquote></div><br></div></div>