[PATCH] D65350: [DDG] Data Dependence Graph Basics

[Bardia via Phabricator via llvm-commits]

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

Addressed comments from Michael and Min-Yih.



================
Comment at: llvm/include/llvm/Analysis/DDG.h:193
+protected:
+  /// Populate the data dependency graph given a set of basic-blocks.
+  virtual void populateGraph(const BasicBlockList &BBs) = 0;
----------------
Meinersbur wrote:
> [serious] Please write something about the algorithmic complexity of this.
Sure. I have put a comment on the `populate` function of `AbstractDependenceGraphBuilder` class.


================
Comment at: llvm/include/llvm/Analysis/DDG.h:212-213
+public:
+  using NodeType = DDGNode;
+  using EdgeType = DDGEdge;
+
----------------
Meinersbur wrote:
> [style] Most often the LLVM uses the `Ty` suffix for typedefs/using.
If I change the suffix only for the usings, they would look inconsistent with the template type names. For example we'll have:
```
template <class GraphType> class AbstractDependenceGraphBuilder {
  using NodeTy = typename GraphType::NodeTy;
```

Notice `GraphType` vs `NodeTy`. Unless you'd like me to rename the template type names too, I think it looks and reads better the way it is right now. By the way, I couldn't find anything on this topic in the LLVM Coding Standards. 


================
Comment at: llvm/include/llvm/Analysis/DDG.h:229
+  /// Populate the data dependency graph given a set of basic-blocks.
+  void populateGraph(const BasicBlockList &BBs) final override;
+};
----------------
Meinersbur wrote:
> [suggestion] Instead of marking the method as final, maybe mark the entire class as final?
I will remove this function.


================
Comment at: llvm/include/llvm/Analysis/DDG.h:273
+public:
+  using Result = std::unique_ptr<DataDependenceGraph>;
+  Result run(Loop &L, LoopAnalysisManager &AM, LoopStandardAnalysisResults &AR);
----------------
Meinersbur wrote:
> [style] `ResultTy`?
The pass manager interface requires each pass to have a type named `Result`. eg in include/llvm/IR/PassManagerInternal.h:

`AnalysisResultModel<IRUnitT, PassT, typename PassT::Result`


================
Comment at: llvm/include/llvm/Analysis/DDG.h:298
+template <> struct GraphTraits<DDGNode *> {
+  using NodeRef = DDGNode *;
+
----------------
Meinersbur wrote:
> This type is not a reference, but a pointer.
Again, the templated implementation of `GraphTraits` and related iterators require this type to be named `NodeRef`. Please see llvm/ADT/GraphTraits.h and llvm/ADT/SCCIterator.h for example.


================
Comment at: llvm/lib/Analysis/DDG.cpp:133
+  for (auto &BB : F.getBasicBlockList())
+    BBList.push_back(const_cast<BasicBlock *>(&BB));
+  populateGraph(BBList);
----------------
Meinersbur wrote:
> Could you either pass a non-const `Function` or use a list of `constBasicBlock`?
Unfortunately many of LLVM interfaces take non-const `Instruction`s (including `DependenceInfo::depends`), so even if I change the list types to be lists of const BasicBlocks and const Instructions, we would need to do a const_cast at some point to interface with the rest of LLVM. I'll change the type of the parameter `F` to be a non-const `Function` :(


================
Comment at: llvm/lib/Analysis/DDG.cpp:134
+    BBList.push_back(const_cast<BasicBlock *>(&BB));
+  populateGraph(BBList);
+}
----------------
Meinersbur wrote:
> [subjective] This is a good illustration of one of the reasons why I dislike doing more than just initializing an object inside its constructor. When calling a virtual function, it will not call the overriden function of the class that will be returned, but the function that it statically resolves to.
> 
> In this case, the `populateGraph` method is marked as final, so there cannot be overriden in another subclass. But then, there is no point in making `populateGraph` virtual as it is protected and called nowhere else, not even in the base class.
> 
> My suggestion is to let a method of `DDGBuilder` return a populated graph instead of doing it in the constructor.
Fair enough, I'll remove the populateGraph function all together.


================
Comment at: llvm/lib/Analysis/DDG.cpp:138-140
+    : DependenceGraphInfo(Twine(L.getHeader()->getParent()->getName() + "." +
+                                L.getHeader()->getName())
+                              .str(),
----------------
Meinersbur wrote:
> [suggestion] Instead of the long initializer expression, assign it inside the body?
Please note that I'm trying to call one of the base class constructors that takes arguments. I think the only way to call it without also calling the default constructor is to use the initializer list.


================
Comment at: llvm/lib/Analysis/DDG.cpp:157-159
+  if (!DDGBase::addNode(N))
+    return false;
+  return true;
----------------
Meinersbur wrote:
> [style] `return DDGBase::addNode(N)`
actually this function does nothing more than calling the base class addNode until we add the pi-blocks, so I'll remove it from this patch.


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:119
+        for (auto &IDst : DstIList) {
+          auto D = const_cast<DependenceInfo &>(DI).depends(ISrc, IDst, true);
+          if (!D)
----------------
Meinersbur wrote:
> Why this `const_cast`? Can we avoid it?
`AbstractDependenceGraphBuilder` holds  a constant reference to the result of the dependence analysis (`DI`). Unfortunately `DependenceInfo::depends` is not marked as constant although it doesn't modify anything. A few ways to solve this:

1. Make `DI` a non-const reference.
2. Do a const_cast when calling `depends`. 
3. Make depends constant (can go viral and is beyond the scope of this work)

I personally prefer (2) as it keeps the const_cast where the const chain broke down. If `depends` is improved and made `const` we just need to drop this const_cast, and in the meantime we prevent code changes that inadvertently modify `DI` in this implementation.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65350