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

[Brian Rzycki via Phabricator via llvm-commits]

brzycki marked 16 inline comments as done.
brzycki added a comment.

@kuba Thank you again for the careful review. I'm uploading a new diff shortly. If you could give it a quick LGTM I'll try to push the patch to tip again today.



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


================
Comment at: llvm/unittests/IR/DeferredDominanceTest.cpp:57
+  DeferredDominance DDT(DT);
+  DDT.flush().verifyDomTree();
+
----------------
kuhar wrote:
> 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.
Done. I used `ASSERT_TRUE(DDT.flush().verify())` because `ASSERT()` isn't a valid macro.


================
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);
----------------
kuhar wrote:
> `!isa<UnreachableInst>` would be more readable, or `ASSERT_NOT(isa<...>(...))`, if the library supports is.
I'll use `isa<>` with the `ASSERT_TRUE()` and `ASSERT_FALSE()` macros.


https://reviews.llvm.org/D40146