[PATCH] D129951: adds `__disable_adl` attribute

Wed Mar 8 14:54:26 PST 2023

cjdb added a comment.

In D129951#4178949 <https://reviews.llvm.org/D129951#4178949>, @philnik wrote:

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

Abseil already doesn't support calling unqualified functions, so this is a QoI improvement, rather than a correctness one like `std::ranges::next`. There's no portability issue because it's just a compatibility rule <https://abseil.io/about/compatibility> of using Abseil that can now get checked at compile time.

  #if defined(__clang__) and __clang_major__ > 18
  #  define ABSL_DISABLE_ADL __disable_adl
  #else
  #  define ABSL_DISABLE_ADL
  #endif

  namespace absl {
    template<class T>
    ABSL_DISABLE_ADL void func(T);
  }

The reason Abseil can get away with it while libc++ cannot is because Abseil doesn't need to adhere to a standard saying "ADL shall not be involved", but rather has the attitude "if you want to painlessly use our library, always qualify your calls to our functions".

>>> 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?

Potentially, but not doing work is a better situation to be in than doing work and then having to do //more// work to evaluate whether or not to keep it.

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

I'm deeply disappointed that libc++ moved away from using `__function_like`: that was an important part of preventing niebloid misuse. It isn't conforming to treat niebloids as function objects, which is what `__function_like` prevented (I just checked `std::ranges::next` and it seems that `__function_like` was completely removed).

> 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?

Each line of the AST dump should represent a branch in the tree.

Notice that even if [we delete the entirety of `main`](https://godbolt.org/z/3zKza3T5P), you'll notice that the AST is still double the size when using structs over functions (since libc++ is no longer using `__function_like`, I've removed that from the linked comparison, but it clocked in at ~+1000 lines of AST). That's because the structs still need to be a part of the AST regardless of use.

> shouldn't it be possible to overcome memory usage by modeling Niebloids in a different way than normal classes?

I think this would require a significant overhaul of how Clang processes structs.

>> 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