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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour marked an inline comment as done.
bmahjour added inline comments.


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:140
+        // array.
+        enum EKind { DefUse, Memory, Rooted, EKCount };
+        bool EdgeAlreadyCreated[DirectionCount][EKCount] = {
----------------
Meinersbur wrote:
> 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.
I think I understand your concern now. I like the idea of a custom factory for creating all types of edges that the client cares about without making the builder know about those edges, however I think a default implementation that only handles DefUse, Memory and Rooted edges would be useful. Do you agree? 
I'll work on it and upload a patch soon.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68827