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

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

@dberlin Any objections to me committing this code?



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


================
Comment at: llvm/include/llvm/IR/DeferredDominance.h:127
+  /// \brief Debug method to help view the state of pending updates.
+  void dump();
+
----------------
kuhar wrote:
> Shouldn't this be guarded by a macro? `LLVM_DUMP_METHOD`
> Can dump() be marked as const?
I wasn't aware of these wrapper macros for dump, thanks for pointing them out. Added.


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


================
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});
----------------
kuhar wrote:
> I think we can reserve (2 * size(successors(SplitBB)) + 3 here to have a single allocation
I did this and also added others where the size was trivial to calculate.


https://reviews.llvm.org/D40146