[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;
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116610