[PATCH] D48392: [Dominators] Simplify child lists and make them deterministic

[Benjamin Kramer via Phabricator via llvm-commits]

bkramer marked an inline comment as done.
bkramer added inline comments.


================
Comment at: include/llvm/Support/GenericDomTreeConstruction.h:1221
     // that case.
     SmallDenseMap<std::pair<NodePtr, NodePtr>, int, 4> Operations;
     Operations.reserve(AllUpdates.size());
----------------
kuhar wrote:
> Won't this also introduce non-determinism in the case you described?
It would, but we restore a deterministic order below by sorting back to input order.


================
Comment at: include/llvm/Support/GenericDomTreeConstruction.h:1255
 
     llvm::sort(Result.begin(), Result.end(),
                [&Operations](const UpdateT &A, const UpdateT &B) {
----------------
kuhar wrote:
> Related to above.
Right, this makes the order deterministic again.


================
Comment at: include/llvm/Support/GenericDomTreeConstruction.h:1272
+           FS.back().getInt() == CurrentUpdate.getKind());
+    FS.pop_back();
     if (FS.empty()) BUI.FutureSuccessors.erase(CurrentUpdate.getFrom());
----------------
kuhar wrote:
> I think this deserves a comment -- it's only obvious that the successor/predecessor is at the end once you realize updates are performed in the same order as they happened to be legalized.
Done.


Repository:
  rL LLVM

https://reviews.llvm.org/D48392