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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/Analysis/DDG.h:68
+  /// true if at least one instruction was collected, and false otherwise.
+  bool CollectInstructions(std::function<bool(Instruction *)> const &Pred,
+                           InstructionList &IList) const;
----------------
[style] By LLVM's coding standard, method names start with lower case.


================
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;
----------------
[serious] Please write something about the algorithmic complexity of this.


================
Comment at: llvm/include/llvm/Analysis/DDG.h:212-213
+public:
+  using NodeType = DDGNode;
+  using EdgeType = DDGEdge;
+
----------------
[style] Most often the LLVM uses the `Ty` suffix for typedefs/using.


================
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;
+};
----------------
[suggestion] Instead of marking the method as final, maybe mark the entire class as final?


================
Comment at: llvm/include/llvm/Analysis/DDG.h:273
+public:
+  using Result = std::unique_ptr<DataDependenceGraph>;
+  Result run(Loop &L, LoopAnalysisManager &AM, LoopStandardAnalysisResults &AR);
----------------
[style] `ResultTy`?


================
Comment at: llvm/include/llvm/Analysis/DDG.h:298
+template <> struct GraphTraits<DDGNode *> {
+  using NodeRef = DDGNode *;
+
----------------
This type is not a reference, but a pointer.


================
Comment at: llvm/lib/Analysis/DDG.cpp:32
+  if (isa<SimpleDDGNode>(this)) {
+    for (auto *I : (cast<const SimpleDDGNode>(this))->getInstructions())
+      if (Pred(I))
----------------
[nit] Parens around cast unnecessary


================
Comment at: llvm/lib/Analysis/DDG.cpp:133
+  for (auto &BB : F.getBasicBlockList())
+    BBList.push_back(const_cast<BasicBlock *>(&BB));
+  populateGraph(BBList);
----------------
Could you either pass a non-const `Function` or use a list of `constBasicBlock`?


================
Comment at: llvm/lib/Analysis/DDG.cpp:134
+    BBList.push_back(const_cast<BasicBlock *>(&BB));
+  populateGraph(BBList);
+}
----------------
[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.


================
Comment at: llvm/lib/Analysis/DDG.cpp:138-140
+    : DependenceGraphInfo(Twine(L.getHeader()->getParent()->getName() + "." +
+                                L.getHeader()->getName())
+                              .str(),
----------------
[suggestion] Instead of the long initializer expression, assign it inside the body?


================
Comment at: llvm/lib/Analysis/DDG.cpp:157-159
+  if (!DDGBase::addNode(N))
+    return false;
+  return true;
----------------
[style] `return DDGBase::addNode(N)`


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:39
+  assert(IMap.empty() && "Expected empty instruction map at start");
+  for (auto *BB : BBList)
+    for (auto &I : *BB) {
----------------
[style] LLVM does not make use of almost-always auto.


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:119
+        for (auto &IDst : DstIList) {
+          auto D = const_cast<DependenceInfo &>(DI).depends(ISrc, IDst, true);
+          if (!D)
----------------
Why this `const_cast`? Can we avoid it?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65350