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

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

I always prefer deterministic execution (all other things being equal). Have you done any testing of performance impact to this change? The adding and erasing of Dominator elements may cost more with how the SmallVector type performs the operations.

Otherwise I think the patch looks good. However, @kuhar is the Dominator expert here and I'd recommend waiting for him to speak up before committing.

> I didn't manage to get the 240 MB IR file reduced enough, triggering this bug requires a lot of jump threading, so landing this without a test case.

Did you try bugpoint? a 240MB IR fire is indeed huge but maybe letting it run over night can help reduce the problem space. Non-deterministic errors like these are very difficult to narrow down though.



================
Comment at: include/llvm/Support/GenericDomTreeConstruction.h:86
+    DenseMap<NodePtr, SmallVector<NodePtrAndKind, 1>> FutureSuccessors;
+    DenseMap<NodePtr, SmallVector<NodePtrAndKind, 1>> FuturePredecessors;
     // Remembers if the whole tree was recalculated at some point during the
----------------
Why did you reduce the initialization size from 4 elements down to 1?


Repository:
  rL LLVM

https://reviews.llvm.org/D48392