[PATCH] D116610: [ADCE][NFC] Batch DT updates together

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/ADCE.cpp:582
   bool Changed = false;
+  SmallVector<DominatorTree::UpdateType, 16> DeletedEdges;
 
----------------
qcolombet wrote:
> kuhar wrote:
> > How did you pick this constant? 
> Randomly to be honest.
> 
> It was 4 for the incremental updates and I figured we need to increase this size while batching more updates together while still hitting the fast path of the container.
Could you either generate some statistics based on some real-world code bases and the pathological test cases mentioned in the patch description, or leave the constant out and let it SmallVector pick the small size automatically?

If you need test bitcode, I have some old whole-program bitcode laying around here: https://drive.google.com/drive/folders/1VJpym19cW-8BVgdtl2MsD3zB4CoEQ93O. You can just feed it to opt.


================
Comment at: llvm/lib/Transforms/Scalar/ADCE.cpp:636
 
+  if (!DeletedEdges.empty())
+    DomTreeUpdater(DT, &PDT, DomTreeUpdater::UpdateStrategy::Eager)
----------------
qcolombet wrote:
> kuhar wrote:
> > How about we move this as an early exit in `DomTreeUpdater::applyUpdates`?
> It is, more or less, already there (this is buried in the domtreeconstruction update code), I just didn't want to assume anything on the client side, plus we save on the instantiation of the DomTreeUpdater all together.
> 
> The first iteration of the patch didn't have this check.
> Put differently, I am happy to get rid of it if you believe it's not helpful.
I think it's fine as-is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116610/new/

https://reviews.llvm.org/D116610