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

[Daniel Berlin via llvm-commits]

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.


On Thu, Jun 16, 2016 at 2:12 PM, Daniel Berlin <dberlin at dberlin.org> wrote:

>
>
> 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/4fde85d0/attachment.html>
-------------- next part --------------
diff --git a/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp b/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
index 35f2f2c..d0a1f93 100644
--- a/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
+++ b/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
@@ -97,28 +97,18 @@ using namespace llvm;
 //===----------------------------------------------------------------------===//
 
 namespace {
-class MergedLoadStoreMotion : public FunctionPass {
+class MergedLoadStoreMotion {
   AliasAnalysis *AA;
   MemoryDependenceResults *MD;
 
 public:
-  static char ID; // Pass identification, replacement for typeid
-  MergedLoadStoreMotion()
-      : FunctionPass(ID), MD(nullptr), MagicCompileTimeControl(250) {
-    initializeMergedLoadStoreMotionPass(*PassRegistry::getPassRegistry());
+  MergedLoadStoreMotion(AliasAnalysis *AA, MemoryDependenceResult *MD) {
+      : AA(AA), MD(MD), MagicCompileTimeControl(250) {
   }
 
-  bool runOnFunction(Function &F) override;
+  bool runOnFunction(Function &F):
 
 private:
-  // This transformation requires dominator postdominator info
-  void getAnalysisUsage(AnalysisUsage &AU) const override {
-    AU.setPreservesCFG();
-    AU.addRequired<AAResultsWrapperPass>();
-    AU.addPreserved<GlobalsAAWrapperPass>();
-    AU.addPreserved<MemoryDependenceWrapperPass>();
-  }
-
   // Helper routines
 
   ///
@@ -152,7 +142,6 @@ private:
   const int MagicCompileTimeControl;
 };
 
-char MergedLoadStoreMotion::ID = 0;
 } // anonymous namespace
 
 ///
@@ -556,13 +545,6 @@ bool MergedLoadStoreMotion::mergeStores(BasicBlock *T) {
 /// \brief Run the transformation for each function
 ///
 bool MergedLoadStoreMotion::runOnFunction(Function &F) {
-  if (skipFunction(F))
-    return false;
-
-  auto *MDWP = getAnalysisIfAvailable<MemoryDependenceWrapperPass>();
-  MD = MDWP ? &MDWP->getMemDep() : nullptr;
-  AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();
-
   bool Changed = false;
   DEBUG(dbgs() << "Instruction Merger\n");