[PATCH] D40146: [JumpThreading] Preservation of DT and LVI across the pass

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

I'm a little bit worried that the tests depend on jumpthreading and it's not obvious what the sequence of operations on DDT is. It's difficult to come up with CFG for all the edge cases and I think it would be better to have something equivalent to the tests for DT and PDT incremental updates.
I'm OK with reapplying this, but when it causes a couple more crashes I'd strongly consider some more exhaustive testing approach.



================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:669
+      // This predecessor of PredBB may already have DestBB as a successor.
+      if (std::find(succ_begin(*I), succ_end(*I), DestBB) == succ_end(*I))
+        Updates.push_back({DominatorTree::Insert, *I, DestBB});
----------------
brzycki wrote:
> I did not see this fail in the specific case of the Chrome assert(). However, it is entirely possible on this code path for the same issue to occur which is why I added the check.
Looks reasonable.

You can use llvm::find here, something like this `if (llvm::find(successors(*I), DestBB) == succ_end(*I))`


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:975
+      // This predecessor of BB may already have Succ as a successor.
+      if (std::find(succ_begin(*I), succ_end(*I), Succ) == succ_end(*I))
+        Updates.push_back({DominatorTree::Insert, *I, Succ});
----------------
brzycki wrote:
> This check is the fix for  the assert() discovered by @rnk . The shape of the IR had an existing edge from *I to Succ. Inserting was incorrect since the DT already had a pre-existing edge. It caused the balancing of Insertions/Deletions to be incorrect.
Just like above.


================
Comment at: llvm/test/Transforms/JumpThreading/ddt-crash.ll:268
+
+define void @chrome(%struct.aaa* noalias sret %arg) local_unnamed_addr #0 align 2 personality i8* bitcast (i32 (...)* @chrome2 to i8*) {
+bb:
----------------
brzycki wrote:
> The reduced testcase that crashed LLVM discovered when compiling Chrome.
Wouldn't it be better to add it in a separate file?


https://reviews.llvm.org/D40146