[PATCH] D48202: Generalize MergeBlockIntoPredecessor. Replace uses of MergeBasicBlockIntoOnlyPred.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea marked 3 inline comments as done.
asbirlea added inline comments.


================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:169
+    Updates.reserve(1 + (2 * succ_size(BB)));
+    Updates.push_back({DominatorTree::Delete, PredBB, BB});
+    for (auto I = succ_begin(BB), E = succ_end(BB); I != E; ++I) {
----------------
kuhar wrote:
> asbirlea wrote:
> > dberris wrote:
> > > Consider `Updates.emplace_back(DominatorTree::Delete, PredBB, BB)` instead? Or does that not work because DominatorTree::UpdateType is an aggregate?
> > Being an aggregate should still work, but it doesn't link because it can't find Delete, declared as static constexpr in DominatorTreeBase. The options I got are:
> > - Instead of DominatorTree::Delete use the "original" enum which is DomTreeBuilder::UpdateKind::Delete, but that's icky to expose.
> > - Use a temporary local variable `auto tmp = DominatorTree::Delete`, and use that in the emplace_back.
> > - Use push_back.
> > I'm inclined to just keep the push_back here, but let me know if you think otherwise.
> FYI: `Updates.push_back({DominatorTree::Delete, PredBB, BB});` is used in almost all places that do DT updates. I'm not sure what benefit emplace_back would have over push_back in this case.
Ack, thank you for the input! Leaving as is.


Repository:
  rL LLVM

https://reviews.llvm.org/D48202