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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

`createPiBlocks` should not handle every case with its own boilerplate code.



================
Comment at: llvm/lib/Analysis/DDG.cpp:40
+  } else if (isa<PiBlockDDGNode>(this)) {
+    for (auto *PN : cast<const PiBlockDDGNode>(this)->getNodes()) {
+      assert(!isa<PiBlockDDGNode>(PN) && "Nested PiBlocks are not supported.");
----------------
[style] Unless the type is too complex, try to avoid `auto`.


================
Comment at: llvm/lib/Analysis/DDG.cpp:84
+    unsigned Count = 0;
+    for (auto *N : Nodes)
+      OS << *N << (++Count == Nodes.size() ? "" : "\n");
----------------
[style] Unless the type is too complex, try to avoid `auto`.


================
Comment at: llvm/lib/Analysis/DDG.cpp:222
+  if (Pi)
+    for (auto *NI : Pi->getNodes())
+      PiBlockMap.insert(std::make_pair(NI, Pi));
----------------
[style] Unless the type is too complex, try to avoid `auto`.


================
Comment at: llvm/lib/Analysis/DDG.cpp:238
 raw_ostream &llvm::operator<<(raw_ostream &OS, const DataDependenceGraph &G) {
   for (auto *Node : G)
+    // Avoid printing nodes that are part of a pi-block twice. They will get
----------------
[style] Unless the type is too complex, try to avoid `auto`.


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:102
+    if (SCC.size() > 1)
+      ListOfSCCs.push_back(NodeListType(SCC.begin(), SCC.end()));
+  }
----------------
[suggestion] Use `ListsOfSCCs.emplace_back`?


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:105
+
+  for (auto &NL : ListOfSCCs) {
+    LLVM_DEBUG(dbgs() << "Creating pi-block node with " << NL.size()
----------------
[style] Unless the type is too complex, try to avoid auto.


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:109
+
+    auto &PiNode = createPiBlock(NL);
+    ++TotalPiBlockNodes;
----------------
[style] Unless the type is too complex, try to avoid `auto`.


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:124
+
+      for (auto *SCCNode : NL) {
+
----------------
[style] Unless the type is too complex, please try to avoid `auto`.


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:140
+        // array.
+        enum EKind { DefUse, Memory, Rooted, EKCount };
+        bool EdgeAlreadyCreated[DirectionCount][EKCount] = {
----------------
[serious] Should use `DDGEdge::EdgeKind`


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:158
+          Src->findEdgesTo(*Dst, EL);
+          for (auto *OldEdge : EL) {
+            if (static_cast<EdgeType *>(OldEdge)->isDefUse()) {
----------------
[style] Unless the type is too complex, please try to avoid `auto`.


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:159
+          for (auto *OldEdge : EL) {
+            if (static_cast<EdgeType *>(OldEdge)->isDefUse()) {
+              if (Dir == Direction::Incoming) {
----------------
AFAICS, `OldEdge` is already of type `EdgeType*`, i.e. the static_cast is unnecessary.


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:159
+          for (auto *OldEdge : EL) {
+            if (static_cast<EdgeType *>(OldEdge)->isDefUse()) {
+              if (Dir == Direction::Incoming) {
----------------
Meinersbur wrote:
> AFAICS, `OldEdge` is already of type `EdgeType*`, i.e. the static_cast is unnecessary.
[serious] Isn't there a way to simplify this chain of ifs? Such as
```
  Kind = OldEdge->getKind();
  bool &AlreadyCreated = EdgeAlreadyCreated[Dir][Kind];
  if (!AlreadyCreated) 
    createEdgeOfKind(*Src, *New, Kind);
    AlreadyCreated = true;
```


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68827