<div dir="ltr">To give you an example, if you applied the attached patch, plus your *conversion* patch (without the refactoring) to the current MLSM, and replaced your runMergedLoadStoreMotion with creating MergedLoadStoreMotion objects and calling runOnFunction, you'd get the same result without needing to refactor the entire file.<div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 16, 2016 at 2:12 PM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">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>On Thu, Jun 16, 2016 at 9:48 AM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org" target="_blank">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></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><span class="HOEnZb"><font color="#888888">
--<br>
Davide<br>
</font></span></blockquote></div><br></div></div>
</blockquote></div><br></div>