[PATCH] D72350: [DDG] Data Dependence Graph - Graph Simplification

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/include/llvm/Analysis/DDG.h:394
+  /// the consecutive instructions after merging belong to the same basic block.
+  bool nodesAreMergeable(const DDGNode &Src,
+                         const DDGNode &Tgt) const final override;
----------------
Meinersbur wrote:
> [style] [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | Function names should start with the verb ]]: `areMergableNodes`
[bikeshedding] `areNodesMergable`?


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:415-417
+      if (TargetInDegreeMap.find(Tgt) == TargetInDegreeMap.end())
+        continue;
+      ++TargetInDegreeMap[Tgt];
----------------
[style] This does two DenseMap lookups. Did you consider using the iterator returned by `DenseMap::find` to access the element?


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:431
+    NodeType &Src = **CandidateSourceNodes.begin();
+    CandidateSourceNodes.erase(&Src);
+    assert(Src.getEdges().size() == 1 &&
----------------
[serious] Since `DenseMap::erase` replaces the item with a tombstone, searching for the next non-tombstone (`DenseMap::begin`) will be linear in time. That is, this while-loop takes O(n^2) just for iterating over the elements.

Maybe have a separate worklist+hastable for marking elements already processed?


================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:453
+
+    // The cancidate list is being used as both a worklist and a lookup table.
+    // If the target node is in the candidate list itself, we need to put the
----------------
[typo] can**c**idate


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72350