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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/include/llvm/ADT/EnumeratedArray.h:21
+template <typename ValueType, typename Enumeration,
+          Enumeration LargestEnum = Enumeration::Count,
+          typename IndexType = int,
----------------
Meinersbur wrote:
> Thanks a lot!
> 
> Could we change `Enumeration::Count` to `Enumaration::Last`? The reason is that `::Count` introduces a new element that compilers warn about if unhanded in switch statements. That is, instead of
> ```
>   enum class E {
>     One,
>     Two,
>     Count
>   };
> 
>   switch (e) {
>   case E::One:
>   case E::Two:
>     break;
>   }
> ```
> `warning: enumerator 'E::Count' in switch of enum 'E' is not handled`
> 
> use 
> 
> ```
>   enum class E {
>     One,
>     Two,
>     Last = Two
>   };
> 
>   switch (e) {
>   case E::One:
>   case E::Two:
>     break;
>   }
> ```
Sure, will do.


================
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:
> > > > > 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 };
> > ...
> > };
> > 
> > ```
> > 
> Within `AbstractDependenceGraphBuilder<GraphType>::createPiBlocks`, only one `EdgeType = typename GraphType::EdgeType` would ever be valid.
> 
> Do you foresee functions using DDG and PDG nodes/edges at the same time? Even if so, mixing scoped enum values of different type is an error, except when casted to get an array index, which use case can be made type safe with [[ https://stackoverflow.com/questions/12927951/array-indexing-converting-to-integer-with-scoped-enumeration#answer-35186573 | EnumeratedArray ]]. I think EnumeratedArray would be a great addition to ADT.
Ok, I've added EnumeratedArray and changed EdgesAlreadyCreated to use it.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68827