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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour marked 2 inline comments as done.
bmahjour added inline comments.


================
Comment at: llvm/include/llvm/Analysis/DependenceGraphBuilder.h:115
+  /// and false otherwise.
+  virtual bool shouldCreatePiBlocks() const { return true; }
+
----------------
etiotto wrote:
> When would be not desired to create pi-blocks?  Is this member function really at this point? 
I added this API to allow client code to control creation of pi-blocks for debugging and experimentation purposes. Furthermore since the graph builder is meant to be more generic than just a DDG builder, having the option to control certain steps of the graph build may be useful in future, for example when we use it to create PDG or other types of dependence graphs where certain steps do not apply.


================
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.");
----------------
etiotto wrote:
> 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.
There is more to be considered than the map if nodes are being removed from pi-blocks. Removing a node from the pi-block may actually remove the cycle or change the SCC, but I cannot think of a practical scenario where doing so would be necessary. Do you have a practical scenario in mind?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68827