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

Brian Rzycki via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 20 13:27:09 PDT 2018


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





More information about the llvm-commits mailing list