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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour marked 17 inline comments as done.
bmahjour added a comment.

Addressed all comments.



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


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:140
+        // array.
+        enum EKind { DefUse, Memory, Rooted, EKCount };
+        bool EdgeAlreadyCreated[DirectionCount][EKCount] = {
----------------
Meinersbur wrote:
> [serious] Should use `DDGEdge::EdgeKind`
The reason I introduced a local enum in this implementation without exposing it to other parts is two fold:

1. In order to avoid type casting the enum to an integral type that can be used as an index into the boolean array, the enum used here should be an unscoped enum. The DDGEdge::EdgeKind enum is a scoped enum and it should stay that way.
2. This builder is supposed to build other types of graphs too, so it's better not to rely on the implementation specifics of one graph (DDG) to build other types of graphs (such as PDG). For example, while the def-use, memory and rooted edges are common concepts between DDG and PDG, the edge names, number of edge kinds or their orderings might be different.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68827