<div dir="ltr">Thanks so much. I hope you don't take it personally :)<div><br></div><div>In this case, i think it would have been *much* easier to simply remove the pass related stuff from the mergedloadstoremotion class (Ie the inheritance from functionpass, getanalysisusage, etc), create an object in the legacy and new pass wrapper, and use it.</div><div><br></div><div><br></div><div>IE</div><div>PreservedAnalyses</div><div>MergedLoadStoreMotionPass::run(Function &F, AnalysisManager<Function> &AM) {</div><div> auto *AA = &AM.getResult<AAManager>(F);</div><div> auto *MD = AM.getCachedResult<MemoryDependenceAnalysis>(F);</div><div> DominatorTree *DT = &AM.getResult<DominatorTreeAnalysis>(F);</div><div> MemorySSA *MSSA = nullptr;</div><div> if (UseMemorySSA) {</div><div> MSSA = &AM.getResult<MemorySSAAnalysis>(F);</div><div> }</div><div> MergedLoadStoreMotion MLSM(AA, MD, MSSA, DT);</div><div> </div><div> if (!MLSM.runOnFunction(F))</div><div> return PreservedAnalyses::all();</div><div> return PreservedAnalyses::none();</div><div>}</div><div><br></div><div><br></div><div>and</div><div> bool runOnFunction(Function &F) override {</div><div> if (skipFunction(F))</div><div> return false;</div><div><br></div><div> AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();</div><div> auto *MDWP = getAnalysisIfAvailable<MemoryDependenceWrapperPass>();</div><div> MD = MDWP ? &MDWP->getMemDep() : nullptr;</div><div> DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();</div><div> MSSA = &getAnalysis<MemorySSAWrapperPass>().getMSSA();</div><div> MergedLoadStoreMotion MLSM(AA, MD, MSSA, DT);</div><div> return MLSM.runOnFunction(F);</div><div> }</div><div><br></div><div>(for the legacy pass).</div><div><br></div><div><br></div><div>Since we are supposed to be getting rid of the legacy stuff eventually, one would presume eventually the MLSM class will just become a pass class again (who knows though :P)</div><div><br></div><div>Excuse the extra pieces, i'm actually almost all the way to making this just work (albeit with the memoryssa merge on top of it)</div><div><br></div><div>This should be a trivial modification of your actual porting patch, but not require this refactoring.</div><div><br></div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 16, 2016 at 10:09 AM, Davide Italiano <span dir="ltr"><<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</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:20 AM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>> wrote:<br>
> (and note, you didn't explain, anywhere, why any of these changes are<br>
> necessary for the new PM, they look, at a glance, like completely orthogonal<br>
> refactorings)<br>
><br>
> On Thu, Jun 16, 2016 at 9:07 AM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>> wrote:<br>
>><br>
>> I don't remember seeing this for review, and you've basically taken a very<br>
>> well organized class and completely destroyed its organizational structure<br>
>> by creating a mix of class member functions and random non-class static<br>
>> functions.<br>
>><br>
>> I don't see this as an improvement, even for the new PM.<br>
>> I also don't think we should try to take every single pass that is a class<br>
>> in LLVM and necessarily turn it into a bunch of static functions that pass<br>
>> around what was previously class state.<br>
>><br>
>> I'm going to ask you to revert and actually post a patch for review so<br>
>> this can be actually discussed.<br>
>><br>
<br>
</span>I'm reverting this now. Please let me know if there are other passes<br>
converted by me which you want to see reverted and I'll do it.<br>
<br>
Thanks,<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Davide<br>
<br>
"There are no solved problems; there are only problems that are more<br>
or less solved" -- Henri Poincare<br>
</font></span></blockquote></div><br></div>