[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:
> > > 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.
> If this is just an implementation that have these three kinds of edges, there is even less of a reason to not use the `EdgeKind` that comes with it.
> 
> Do you think of a use case where there is an dependence graph with an EdgeKind that is never used (at least not before `createPiBlocks`?). The worst that happens here is you have unused elements in EdgeAlreadyCreated.
> 
> I generally don't like complicating things before the extra complexity becomes necessary.
> The worst that happens here is you have unused elements in EdgeAlreadyCreated.
Unused elements can happen too, but the worst that can happen is edge kinds getting misinterpreted because they don't end up with the same enumeration value. eg:

```
class DDGEdge : public DDGEdgeBase {
public:
  enum class EdgeKind { Unknown, RegisterDefUse, MemoryDependence, Rooted };
...
};

class PDGEdge : public PDGEdgeBase {
public:
  enum class EdgeKind { Unknown, MemoryDependence, RegisterDefUse, Control, Rooted };
...
};

```



Repository:
  rL LLVM

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

https://reviews.llvm.org/D68827