[PATCH] D68827: [DDG] Data Dependence Graph - Pi Block

[Ettore Tiotto via Phabricator via llvm-commits]

etiotto added inline comments.


================
Comment at: llvm/include/llvm/Analysis/DependenceGraphBuilder.h:115
+  /// and false otherwise.
+  virtual bool shouldCreatePiBlocks() const { return true; }
+
----------------
When would be not desired to create pi-blocks?  Is this member function really at this point? 


================
Comment at: llvm/lib/Analysis/DDG.cpp:16
 
+static cl::opt<bool>
+    CreatePiBlocks("ddg-pi-blocks", cl::init(true), cl::Hidden, cl::ZeroOrMore,
----------------
Is probably overkill to have an option to disable the creation of pi-blocks. At least at this point. Less is more :-)


================
Comment at: llvm/lib/Analysis/DDG.cpp:215
+  // already reachable by root.
+  auto *Pi = dyn_cast<PiBlockDDGNode>(&N);
+  assert(!Root || Pi && "Root node is already added. No more nodes can be added.");
----------------
This makes me think what happens when a node that is part of a pi-block is removed from the graph (via DirectedGraph::removeNode). We should update the PiBlockMap in that case to reflect that the node no longer exists in the graph.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68827