[PATCH] D58373: [Dominators] Avoid potentially quadratic std::is_permutation

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D58373#1412014 <https://reviews.llvm.org/D58373#1412014>, @kuhar wrote:

> In D58373#1406843 <https://reviews.llvm.org/D58373#1406843>, @MaskRay wrote:
>
> > In D58373#1402235 <https://reviews.llvm.org/D58373#1402235>, @kuhar wrote:
> >
> > > `Roots.size()` is only > 1 for the PostDominatorTree, and I'd expect it to be small (< 100) on virtually all functions. `is_permulation` was used in the original code because it doesn't allocate and should be cheaper for small trees.
> > >
> > > Do you have some statistics on the size of Roots? At what size does SmallPtrSet start to outperform `is_permuatation`?
> >
> >
> > Thanks for the empirical data (<100). I find SmallPtrSet<void *, 4> starts to outperform `std::is_permutation` when there are about 16 elements on my machine. It may be 2x slower (should be larger, but I put it in a loop and the loop itself has some costs) when there is only one root. So the benefit to use a `SmallPtrSet` here is just to filter out some unlikely cases (>1000 roots) and to clear up my doubts when I see that `verifyRoots` claims to be O(N)...
>
>
> It seems it would be best to create a small helper function that does permutation check using std::is_permuatation for small arrays and uses SmallPtrSet for the rest.


But isn't that overkill? When there are fewer than 16 elements, the time spent is negligible (and the 2x slowdown). This is to protect pessimistic cases (>1000 roots).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58373