[PATCH] D129951: adds `__disable_adl` attribute

[Nikolas Klauser via Phabricator via cfe-commits]

philnik added a comment.

In D129951#4178844 <https://reviews.llvm.org/D129951#4178844>, @cjdb wrote:

> In D129951#4178154 <https://reviews.llvm.org/D129951#4178154>, @philnik wrote:
>
>> I don't think libc++ can adopt this without having to essentially duplicate our code, since GCC doesn't support `__disable_adl` (and AFAICT there is no coordination between GCC and Clang to add it to both).
>
> I haven't had a lot of time to drive this in Clang, let alone GCC. Even if libc++ can't ultimately use it (which would be sad), there are other libraries that can. For example, Abseil has a similar attitude towards functions as Niebloids, and could wrap it behind a macro.

Abseil has the same support problem though AFAICT. In fact, most open source libraries don't //just// support clang.

>> Have you tested what impact making the members `static` has? Both clang and GCC already support this as an extension back to C++11: https://godbolt.org/z/drE5v8nYo.
>
> A quick change to the original benchmark <https://godbolt.org/z/13z65EY88> shows the AST for `static operator()` being substantially larger than a function template with ADL being disabled. I haven't properly benchmarked build time impact, but here's a quick one <https://gist.github.com/cjdb/6ade504f010dc550890a82f3a5c0ea6a>. The averages are below:
>
> **`__disable_adl`**
>
>   real  0.1164
>   user  0.0706
>   sys   0.0488
>
> **`static operator()`**
>
>   real  0.1272
>   user  0.081
>   sys   0.0488
>
> It is worth acknowledging that the assembly output is now much closer with optimised flags (1.63x larger as opposed to 7.56x larger), but 1.26x larger with `-g` (this is down from 1.66x as non-static).

Couldn't that be overcome with some optimizations for Niebloids?

>> Maybe it would make more sense to add an attribute `[[clang::cpo]]` instead to tell clang that the class should just be treated as an overload set? Make it requirements that the class is empty, there are no non-static member functions and the class is declared `final` and you should get any benefits without the major drawback of basically no portability. It's of course possible that I'm missing something major, but I think that general way would be a lot more acceptable. Any thoughts?
>
> CPOs and Niebloids are different things (and `__disable_adl` is for Niebloids, not CPOs), so any such attribute would need a different name.

Yes. Sorry for the conflation.

> Having said that, a struct that hasn't has no base and is final only slightly improves the AST size <https://godbolt.org/z/ncq1qx5Ys> with respect to the improvement by using an actual overload set. Finally, there would still be a portability issue because even if `[[clang::niebloid]]` works on Clang, there would still need to be coordination for it to work on GCC; otherwise GCC w/ libc++ mode would have copyable Niebloids; something that the original libc++ design worked hard to ensure wasn't possible so that a feature like this could exist.

I don't know about the original design, but at least the algorithms are copyable. I wouldn't be too concerned if that was different between clang and GCC, it's at least conforming in both cases. Regarding AST size, I don't know how representative LoC in the dump are, but shouldn't it be possible to overcome memory usage by modeling Niebloids in a different way than normal classes?

> It is again worth acknowledging that the assembly output in an optimised build would have parity, but a build using `-O0 -g` will still be ~1.26x larger.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129951