[PATCH] D90159: [DDG] Data Dependence Graph - DOT printer

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/Analysis/DDG.h:475
+                                                   const NodeType &Dst) const {
+  std::string Str("");
+  raw_string_ostream OS(Str);
----------------
[nit] The initializer `("")` is useless? `std::string` default-initializes to an empty string.


================
Comment at: llvm/include/llvm/Analysis/DDG.h:480
+    return OS.str();
+  unsigned count = 0;
+  for (auto &D : Deps) {
----------------
[suggestion] Instead of count, you can use `llvm::enumerate`. I think it's nicer to check at for "not the first" at the beginning instead of the last element at the end.

There also is an `interleave` function in STLExtras.h for uses cases like this.


================
Comment at: llvm/include/llvm/Analysis/DDGPrinter.h:21
+#include "llvm/Support/DOTGraphTraits.h"
+namespace llvm {
+
----------------
[style] Space between includes and namespace?


================
Comment at: llvm/include/llvm/Analysis/DDGPrinter.h:45
+    assert(G && "expected a valid pointer to the graph.");
+    Graph = G;
+    return "DDG for '" + std::string(G->getName()) + "'";
----------------
Why does a getter set a field value?


================
Comment at: llvm/lib/Analysis/DDGPrinter.cpp:109
+  if (isa<SimpleDDGNode>(Node))
+    for (auto *II : static_cast<const SimpleDDGNode *>(Node)->getInstructions())
+      OS << *II << "\n";
----------------
Does this const_cast exist to add const qualifier?


================
Comment at: llvm/lib/Analysis/DDGPrinter.cpp:117
+      OS << getVerboseNodeLabel(PN, G)
+         << (++Count == PNodes.size() ? "" : "\n");
+    OS << "--- end of nodes in pi-block ---\n";
----------------
Could you split this up?
```
if (Count != PNodes.size())
  OS << "\n";
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90159