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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/ADCE.cpp:582
   bool Changed = false;
+  SmallVector<DominatorTree::UpdateType, 16> DeletedEdges;
 
----------------
kuhar wrote:
> qcolombet wrote:
> > qcolombet wrote:
> > > kuhar wrote:
> > > > 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.
> > > > Could you either generate some statistics based on some real-world code bases
> > > 
> > > I'll do that thanks for the pointer.
> > > 
> > > > ... the pathological test cases
> > > 
> > > The pathological test cases are less interesting because they involve thousands of edges and that's not a small vector at this point :).
> > Here are some statistics based on the bitcode files you've shared. We delete 733 edges total on the whole suite.
> > Ignoring the cases where we don't remove anything, the number of edges to delete:
> > - Average at 1.5
> > - Median is 1
> > - The max is 36 (happens once) and the next max is 10 (happens 5 times)
> > 
> > All in all, if we leave the size at 4, with that suite, we hit the fast storage in ~97.3% of the cases.
> > If we bump it to 16 (like I did here), we hit the fast storage in ~99.9% of the cases.
> Thanks! I'm surprised the max is so small. <= 10 or the default size sound good to me then.
That was surprising to me too to be honest :).

I got these numbers by running opt with -O3.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116610