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

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

@kuhar if you have the time I would appreciate your feedback on the fix and if you're still ok with the patch.



================
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});
----------------
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.


================
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});
----------------
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.


================
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:
----------------
The reduced testcase that crashed LLVM discovered when compiling Chrome.


https://reviews.llvm.org/D40146