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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

I am still unconvinced over the boilerplate `createPiBlocks`.

This method of de-duplication of edges assumes that edges are only defined by its kind. Is it correct that no additional properties, such as which definition of a value caused a UseDef dependency, are planned?



================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:140
+        // array.
+        enum EKind { DefUse, Memory, Rooted, EKCount };
+        bool EdgeAlreadyCreated[DirectionCount][EKCount] = {
----------------
bmahjour wrote:
> 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.
> 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.

It also means we have to maintain a mapping between the two enums which are basically identical. I'd prefer the cast over this.

One can abstract away the the type-casting into a type-safe class, such as proposed here:
https://stackoverflow.com/questions/12927951/array-indexing-converting-to-integer-with-scoped-enumeration#answer-35186573


> 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.

I cannot follow this argument. `createPiBlocks()` as-is relies on knowing all types of edges since there is no abstraction. When introducing a new type of edge, it will hit `llvm_unreachable("Unsupported edge kind")`. It also cannot create edges of arbitrary kind since it needs to call a `createXYZEdge` method. If you add a kind of edge to PDG only, it will need to call a `createABCEdge` method, which, if not also added to the DDG, will make the `AbstractDependenceGraphBuilder` uninstantiable for the DDG.

If the abstraction of node kind is important, I suggest to add a way to create edges without knowing its type, such as a `clone()` method on edges or promoting `createEdgeOfKind` to become a method of the graph builder such that subclasses can implement custom factories for their edges.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68827