[PATCH] D14677: [LegacyPassManager] Reduce memory usage for AnalysisUsage

[Chandler Carruth via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Mostly minor changes. I'm not thrilled with the folding set stuff still, but i don't want to spend more time tweaking the legacy pass manager code honestly. I'm not even thrilled by this complexity going in, but I understand why it's so significant for your use case.


================
Comment at: include/llvm/IR/LegacyPassManagers.h:257-258
@@ +256,4 @@
+  /// is used to avoid needing to make AnalysisUsage itself a folding set node.
+  class AUFoldingSetNode : public FoldingSetNode {
+  public:
+    // TODO: We could consider sorting the dependency arrays within the
----------------
nit: Just make this a struct to avoid needing to specify "public" everywhere.

================
Comment at: include/llvm/IR/LegacyPassManagers.h:259-260
@@ +258,4 @@
+  public:
+    // TODO: We could consider sorting the dependency arrays within the
+    // AnalysisUsage (since they are conceptually unordered).
+    AnalysisUsage AU;
----------------
This would make a bit more sense below, where we process the arrays?

================
Comment at: include/llvm/IR/LegacyPassManagers.h:273-274
@@ +272,4 @@
+    }
+    static void Profile(FoldingSetNodeID &ID,
+                        const AnalysisUsage::VectorType &Vec) {
+      ID.AddInteger(Vec.size());
----------------
I would prefer to not add to the Profile overload set since that gets called at a distance in the folding set code. Instead, I'd rather make it clearly an implementation detail. Unless I missed something and it's actually used elsewhere?

ProfileImpl would be fine. Maybe even a local lambda?

Actually, what about writing the above as:

  for (auto &Vec : {AU.getRequiredSet(), AU.getRequiredTransitiveSet(), ...}) {
    ID.AddInteger(Vec.size());
    for (AnalysisID AID : Vec)
      ID.AddPointer(AID);
  }

I think we're finally at a point where MSVC's initializer list support is up to this style of usage.

================
Comment at: lib/IR/LegacyPassManager.cpp:595
@@ +594,3 @@
+    }
+    assert(Node);
+
----------------
Add a message here? Maybe "found a null pointer in cached analysis usages" or somesuch?


http://reviews.llvm.org/D14677