[PATCH] D65350: [DDG] Data Dependence Graph Basics

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

> This patch contains support for a basic DDGs containing only atomic nodes (one node for each instruction). The edges are two fold: def-use edges and memory-dependence edges. The idea behind the DependenceGraphBuilder and why we need it are summarized in https://ibm.ent.box.com/v/directed-graph-and-ddg.

I think it would be good to summarize the information in-tree as well, to ensure the information is accessible later on as well. Some of the docs fit in the headers, for some of it a new documentation page might be worth adding. Ideally it would include some info about  design decisions, the intended/example uses cases and how the DDG helps and the benefits over the existing infrastructure.

A few additional questions:

I am not sure I see the direct benefit of duplicating the def-use edges in the DDG? Given a User, we already have access to its uses throughout the User itself and LLVM tries hard to maintain that relation very efficiently.

IIUC the plan is to build the DDG up front and then check the legality of a transformation on top of it. Currently, most passes bail out early once they detect a transformation cannot be applied and this helps to limit compile time. Could we do something similar with checks dependent on the DDG?



================
Comment at: llvm/include/llvm/Analysis/DDG.h:101
+  /// Get the list of instructions in this node.
+  const InstructionList &getInstructions() const {
+    assert(!InstList.empty() && "Instruction List is empty.");
----------------
Unless there is a good reason, I think it would be better to return an ArrayRef/SmallVectorImpl instead of a concrete SmallVector here (and other places, same for arguments).


================
Comment at: llvm/include/llvm/Analysis/DDG.h:199
+  // queried it is recomputed using @DI.
+  const DependenceInfo DI;
+};
----------------
Why do we need a copy here? Wouldn't a reference be enough?


================
Comment at: llvm/include/llvm/Analysis/DDG.h:233
+  DDGNode &createFineGrainedNode(Instruction &I) final override {
+    auto *SN = new SimpleDDGNode(I);
+    assert(SN && "Failed to allocate memory for simple DDG node.");
----------------
Do we need dynamic memory allocation here? Can the DDG just own the nodes?



================
Comment at: llvm/include/llvm/Analysis/DependenceGraphBuilder.h:26
+using BasicBlockList = SmallVector<BasicBlock *, 2>;
+using InstructionList = SmallVector<Instruction *, 8>;
+
----------------
The default seems a bit excessive. Do you expect most nodes to have multiple instructions?


================
Comment at: llvm/include/llvm/Analysis/DependenceGraphBuilder.h:89
+  /// Map types to map instructions to nodes used when populating the graph.
+  using InstToNodeMap = std::unordered_map<Instruction *, NodeType *>;
+
----------------
Is there a reason to not use DenseMap here?	


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65350