[PATCH] D40146: [JumpThreading] Deferred preservation of DT and LVI across the pass.

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

Great work! I don't have time to check the jump-threading, but the dominance part looks solid. I only found a couple of issues.



================
Comment at: llvm/include/llvm/IR/DeferredDominance.h:53
+public:
+  DeferredDominance(DominatorTree *DT_) : DT(DT_) {}
+
----------------
Can the DT ever be null within the class? If not, maybe it would make sense to keep a reference instead?


================
Comment at: llvm/include/llvm/IR/DeferredDominance.h:102
+  /// DominatorTree pointer to be used by the caller for analysis.
+  DominatorTree *flush() {
+    // Updates to DT must happen before blocks are deleted below. Otherwise the
----------------
Maybe it'd make more sense to return a reference?


================
Comment at: llvm/include/llvm/IR/DeferredDominance.h:127
+  /// \brief Debug method to help view the state of pending updates.
+  void dump();
+
----------------
Shouldn't this be guarded by a macro? `LLVM_DUMP_METHOD`
Can dump() be marked as const?


================
Comment at: llvm/lib/IR/Dominators.cpp:402
+
+void DeferredDominance::dump() {
+  raw_ostream &OS = llvm::outs();
----------------
This doesn't seem to modify any internal state, const should work here.


================
Comment at: llvm/lib/IR/Dominators.cpp:403
+void DeferredDominance::dump() {
+  raw_ostream &OS = llvm::outs();
+  OS << "PendUpdates:\n";
----------------
I think this should be llvm::dbgs


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:2474
+    // NewBB and SplitBB are newly created blocks which require insertion.
+    std::vector<DominatorTree::UpdateType> Updates;
+    Updates.push_back({DominatorTree::Insert, BB, SplitBB});
----------------
I think we can reserve (2 * size(successors(SplitBB)) + 3 here to have a single allocation


https://reviews.llvm.org/D40146