[PATCH] D78861: [Attributor] Track AA dependency using dependency graph

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Thanks for working on this. I added a first set of comments below. We'll have to rebase this once the changes to reduce memory usage are all in. We will also need to verify this does not regress memory usage too much. Finally, right now this patch needs two command line options to dump and view the dependence graph. That will also allow tests. I guess we should have a printer for the AbstractAttributes so that we print the context instruction, if present, and the underlying value, the kind and state.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:172
+
+  std::vector<DepRecord> DepAAs;
+
----------------
You have declared `DepAAVector` above, maybe rename it to `DepAAVectorTy` and use it here. Also use a SmallVector. The pair should probably be a PointerIntPair instead. The `public` are not needed. You might want a `private` for the members.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:212
+  }
+};
+
----------------
Use DenseMap instead of std::map.

Do we really need to add all edges from the synthetic root into a map? We can just pretend we did, right? Maybe the graph can take a const vector& containing all abstract attributes and the root node just iterates those as children. I want to avoid the memory overhead here.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2032
 
+void AADepGraph::viewGraph() { llvm::ViewGraph(this, "CallGraph"); }
+
----------------
This is not a call graph.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78861/new/

https://reviews.llvm.org/D78861