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

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar accepted this revision.
kuhar added a comment.

Thanks for the tests, they look good to me. Found only some nits.



================
Comment at: llvm/unittests/IR/DeferredDominanceTest.cpp:10
+
+#include "CFGBuilder.h"
+#include "llvm/AsmParser/Parser.h"
----------------
Unused?


================
Comment at: llvm/unittests/IR/DeferredDominanceTest.cpp:57
+  DeferredDominance DDT(DT);
+  DDT.flush().verifyDomTree();
+
----------------
While unlikely it makes a difference, it would be better to use `ASSERT(DDT.flush().verify())` instead -- it's able to catch much more errors and explain them. And the old verifier asserts upon failure, which can be surprising.


================
Comment at: llvm/unittests/IR/DeferredDominanceTest.cpp:70
+
+  // delete edge bb0 -> bb3
+  // Push the update twice to verify duplicate entries are discarded.
----------------
Nit: s/delete/Delete + missing comma at the end


================
Comment at: llvm/unittests/IR/DeferredDominanceTest.cpp:95
+  // BranchInst::Create() call above.
+  EXPECT_EQ(dyn_cast<UnreachableInst>(BB3->getTerminator()), nullptr);
+  EXPECT_EQ(DDT.pendingDeletedBB(BB3), false);
----------------
`!isa<UnreachableInst>` would be more readable, or `ASSERT_NOT(isa<...>(...))`, if the library supports is.


================
Comment at: llvm/unittests/IR/DeferredDominanceTest.cpp:99
+  EXPECT_EQ(DDT.pendingDeletedBB(BB3), true);
+  EXPECT_NE(dyn_cast<UnreachableInst>(BB3->getTerminator()), nullptr);
+  EXPECT_EQ(BB3->getParent(), F);
----------------
`!isa<UnreachableInst>` would be more readable


================
Comment at: llvm/unittests/IR/DeferredDominanceTest.cpp:105
+  DDT.applyUpdates(Updates);
+  DDT.flush().verifyDomTree();
+}
----------------
Better to use `ASSERT(...verify())`


================
Comment at: llvm/unittests/IR/DeferredDominanceTest.cpp:132
+  DeferredDominance DDT(DT);
+  DDT.flush().verifyDomTree();
+
----------------
verify()


================
Comment at: llvm/unittests/IR/DeferredDominanceTest.cpp:211
+  DDT.recalculate(*F);
+  DDT.flush().verifyDomTree();
+
----------------
verify()


================
Comment at: llvm/unittests/IR/DeferredDominanceTest.cpp:223
+                    {DominatorTree::Insert, NewEntry, BB1}});
+  DDT.flush().verifyDomTree();
+
----------------
verify()


================
Comment at: llvm/unittests/IR/DeferredDominanceTest.cpp:226
+  // Now remove bb0 from F.
+  EXPECT_EQ(dyn_cast<UnreachableInst>(BB0->getTerminator()), nullptr);
+  EXPECT_EQ(DDT.pendingDeletedBB(BB0), false);
----------------
!isa


================
Comment at: llvm/unittests/IR/DeferredDominanceTest.cpp:230
+  EXPECT_EQ(DDT.pendingDeletedBB(BB0), true);
+  EXPECT_NE(dyn_cast<UnreachableInst>(BB0->getTerminator()), nullptr);
+  EXPECT_EQ(BB0->getParent(), F);
----------------
!isa


================
Comment at: llvm/unittests/IR/DeferredDominanceTest.cpp:237
+  DDT.recalculate(*F);
+  DDT.flush().verifyDomTree();
+}
----------------
verify()


================
Comment at: llvm/unittests/IR/DeferredDominanceTest.cpp:306
+  // method converts bb2's TI into "unreachable".
+  EXPECT_EQ(dyn_cast<UnreachableInst>(BB2->getTerminator()), nullptr);
+  EXPECT_EQ(DDT.pendingDeletedBB(BB2), false);
----------------
!isa


================
Comment at: llvm/unittests/IR/DeferredDominanceTest.cpp:329
+  // method converts bb1's TI into "unreachable".
+  EXPECT_EQ(dyn_cast<UnreachableInst>(BB1->getTerminator()), nullptr);
+  EXPECT_EQ(DDT.pendingDeletedBB(BB1), false);
----------------
!isa


================
Comment at: llvm/unittests/IR/DeferredDominanceTest.cpp:333
+  EXPECT_EQ(DDT.pendingDeletedBB(BB1), true);
+  EXPECT_NE(dyn_cast<UnreachableInst>(BB1->getTerminator()), nullptr);
+  EXPECT_EQ(BB1->getParent(), F);
----------------
isa


================
Comment at: llvm/unittests/IR/DeferredDominanceTest.cpp:344
+  DDT.applyUpdates(Updates);
+  DDT.flush().verifyDomTree();
+}
----------------
verify()


https://reviews.llvm.org/D40146