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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

LGTM, with some nitpicks.



================
Comment at: llvm/include/llvm/Analysis/DDG.h:212-213
+public:
+  using NodeType = DDGNode;
+  using EdgeType = DDGEdge;
+
----------------
bmahjour wrote:
> 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. 
It's not in the coding standard document, but as mentioned it is something you see a lot in the existing code base. I find it useful as the compiler would be confused if one writes e.g.
```
using SomeList = SmallVector<...>;
...
SomeList SomeList;
```

However, I see you are trying to match the template argument names (And indeed I don't see the Ty suffix in template argument names in the code base). Therefore, I think your choice makes sense here.


================
Comment at: llvm/include/llvm/Analysis/DDG.h:273
+public:
+  using Result = std::unique_ptr<DataDependenceGraph>;
+  Result run(Loop &L, LoopAnalysisManager &AM, LoopStandardAnalysisResults &AR);
----------------
bmahjour wrote:
> 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`
OK, didn't notice this.


================
Comment at: llvm/include/llvm/Analysis/DDG.h:298
+template <> struct GraphTraits<DDGNode *> {
+  using NodeRef = DDGNode *;
+
----------------
bmahjour wrote:
> 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.
Sorry, didn't see this as a requirement of `GraphTraits`


================
Comment at: llvm/include/llvm/Analysis/DDG.h:107
+    return const_cast<InstructionList &>(
+        static_cast<const SimpleDDGNode *>(this)->getInstructions());
+  }
----------------
[style] Why not
```
return InstrList;
```
instead of those casts?

Additionally, I don't see the harm of returning and empty instruction list. Especially if the function is used to add instructions to the node.


================
Comment at: llvm/include/llvm/Analysis/DDG.h:117
+  /// Define classof to be able to use isa<>, cast<>, dyn_cast<>, etc.
+  static bool classof(const DDGNode *N) {
+    return N->getKind() == NodeKind::SingleInstruction ||
----------------
[suggestion] There often is also `static bool classof(const SimpleDDGNode *) { return true; }` for the case when the compiler already knows the type.


================
Comment at: llvm/include/llvm/Analysis/DependenceGraphBuilder.h:25-26
+
+using BasicBlockList = SmallVector<BasicBlock *, 2>;
+using InstructionList = SmallVector<Instruction *, 8>;
+
----------------
[style] The names "BasicBlockList" and "InstructionList" a very general to be put into the entire `llvm::` namespace and could conflict with other headers that use lists of instructions and BBs. Maybe move them into one of the DDG classes?

Also the `Ty` suffix for typedefs would apply here.


================
Comment at: llvm/lib/Analysis/DDG.cpp:138-140
+    : DependenceGraphInfo(Twine(L.getHeader()->getParent()->getName() + "." +
+                                L.getHeader()->getName())
+                              .str(),
----------------
bmahjour wrote:
> 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.
OK.


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:119
+        for (auto &IDst : DstIList) {
+          auto D = const_cast<DependenceInfo &>(DI).depends(ISrc, IDst, true);
+          if (!D)
----------------
bmahjour wrote:
> 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.
DI is created by this patch in `DDGAnalysis::run` and don't see a reason to pass it as `const &` if the class was not designed for it. That is, I'd prefer (1).


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:102
+  };
+  for (DGIterator SrcIt = Graph.begin(); SrcIt != Graph.end(); ++SrcIt) {
+    InstructionList SrcIList;
----------------
[style] By [[ https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop | coding standard ]] , the end-iterator should be cached (the list of nodes is not modified within the body, right)?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65350