[PATCH] D48919: [Dominators] Convert existing passes to use DomTreeUpdater - 1

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

One thing that is not clear to me is whether it's work to replace plain updates (DT->insert/deleteEdge, DT->applyUpdates) with DTU when it's clear that both are going to be functionally the same. The overhead of constructing DTU is probably negligible, but it's more verbose and I'm not sure what benefit it brings.

It definitely makes sense to use DTU when you are updating both DT and PDT, and when you can use the lazy strategy, or use some other functions that take DTU. Do you think it's worth to replace it everywhere for some other reason?



================
Comment at: lib/Transforms/Scalar/ADCE.cpp:618
 
-    DT.applyUpdates(DeletedEdges);
-    PDT.applyUpdates(DeletedEdges);
+    DomTreeUpdater DTU(DT, PDT, DomTreeUpdater::UpdateStrategy::Eager);
+    DTU.applyUpdates(DeletedEdges);
----------------
Maybe use it directly from the constructor without creating a local variable -- like below? 

I think either is fine - this should be easier to debug, the latter is more concise, but I think it's better to be somewhat consistent. What do you think?


Repository:
  rL LLVM

https://reviews.llvm.org/D48919