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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Analysis/DependenceGraphBuilder.cpp:431
+    NodeType &Src = **CandidateSourceNodes.begin();
+    CandidateSourceNodes.erase(&Src);
+    assert(Src.getEdges().size() == 1 &&
----------------
bmahjour wrote:
> Meinersbur wrote:
> > [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?
> I was trying to avoid a second container to avoid memory waste, but since the worklist is transient and typically small, duplicating it should be fine. I've also found a bug where (depending on the order) merging nodes can trigger the "Expected a single edge from the candidate src node." assert. I've fixed this as well and added comments.
It would have been a good idea of there wasn't the downside of O(n^2).

As you noticed, iterating over a DenseMap can lead to non-deterministic behavior (There's `MapVector` to avoid this).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72350