[libcxx-commits] [PATCH] D108216: [libc++] Formulate _EnableIf in terms of std::enable_if
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Aug 31 08:26:39 PDT 2021
ldionne added inline comments.
================
Comment at: libcxx/include/__functional/bind_front.h:41
-template <class _Fn, class... _Args, class = _EnableIf<
+template <class _Fn, class... _Args, class = enable_if_t<
_And<
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Quuxplusone wrote:
> > > ldionne wrote:
> > > > Quuxplusone wrote:
> > > > > ldionne wrote:
> > > > > > Quuxplusone wrote:
> > > > > > > (1) What do the respective error messages look like with GCC? MSVC? (I bet those compilers lack Clang's special cases.)
> > > > > > > (2) Is it conceivable that we could just make Clang understand anything-of-the-general-form-of-`_EnableIf`, based on the class template's shape, instead of based on special-casing its name?
> > > > > > > (3) If all else fails, could we keep using the spelling `_EnableIf`, and just define `_EnableIf` as an alias for `enable_if_t`? At least then it would be easy for anyone who cared about compile-time performance to just patch it in one place. (Arguably I bet it would do no harm for them to patch it at the `enable_if_t` level, and thus speed up even more things than just libc++ itself.) Also, it would make this patch a lot less invasive.
> > > > > > > (4) Is this the camel's nose in the tent? Are you soon going to propose replacing `_If` with `conditional_t`? Do we want this?
> > > > > >
> > > > > > > (1) What do the respective error messages look like with GCC? MSVC? (I bet those compilers lack Clang's special cases.)
> > > > > >
> > > > > > Indeed, only Clang provides a better diagnostic (but I still think it matters because Clang is by far the most important compiler for libc++. https://godbolt.org/z/4jx35WcrP
> > > > > >
> > > > > > > (2) Is it conceivable that we could just make Clang understand anything-of-the-general-form-of-`_EnableIf`, based on the class template's shape, instead of based on special-casing its name?
> > > > > >
> > > > > > I think we could, although we might end up with false positives. Would you consider any template that doesn't have a nested `::type` and has a `template <bool, class = void>` parameter list to be an `enable_if`? See https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaTemplate.cpp#L10483.
> > > > > >
> > > > > > > (3) If all else fails, could we keep using the spelling `_EnableIf`, and just define `_EnableIf` as an alias for `enable_if_t`? At least then it would be easy for anyone who cared about compile-time performance to just patch it in one place.
> > > > > >
> > > > > > I guess the question I have here is "why do you prefer `_EnableIf` over `enable_if_t`? My mental model is that we strive to use standard utilities when we can, and we use shims like `_EnableIf` (which should really be called `__enable_if_t`) when we need the utility in C++N but it's not standardized until some later C++M version. Just like we have e.g. `__to_address` for pre-C++20 code. I think it would be good to agree on this general question.
> > > > > >
> > > > > > > (4) Is this the camel's nose in the tent? Are you soon going to propose replacing _If with conditional_t? Do we want this?
> > > > > >
> > > > > > I had no plans to do that since I'm not aware of a tangible benefit, however you're right that depending on what we end up agreeing upon for (3), the logical successor to this change could be to also move to `conditional_t` (whether we'll actually do it is a different story).
> > > > > >
> > > > > > Note: I thought about defining `enable_if_t` the same way `_EnableIf` is defined for compile-time performance, however Clang apparently doesn't understand that either: https://godbolt.org/z/jE5T7GxPb. I do think this one would be easier to fix, though. (I'm surprised it doesn't work given this: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaTemplate.cpp#L3514)
> > > > > > (2) Would you consider any template that doesn't have a nested `::type` and has a `template <bool, class = void>` parameter list to be an `enable_if`?
> > > > > Well, remember that our `_EnableIf` has a different shape from the standard `enable_if_t`. Ours is `_MetaBase<B>::_EnableIfImpl<R>`, whereas `enable_if_t` is `enable_if<B, R>::type`. So I think the criterion would be something like "We failed to find a member in a class template of the form `template<[...], bool, [...]>`, where that bool parameter is `false`, //and// where, if the bool parameter had been `true`, we //would// have found such a member." If that's too backtracky (Clang sucks at backtracking/hypotheticals), we could say "We failed to find a member in a class template of the form `template<[...], bool, [...]>`, where that bool parameter is `false`, //and// where the class template has at least one partial or explicit specialization." I would //not// look at the name of the member, nor the name of the class, nor the reason-we're-looking-for-that-member (e.g. the name of the alias template).
> > > > >
> > > > > > Note: I thought about defining `enable_if_t` the same way `_EnableIf` is defined for compile-time performance
> > > > > That would be non-conforming (and also an ABI break). If it were conforming, we'd have done it long ago. Here's why: the alias's expansion is observable by the programmer. https://godbolt.org/z/YeaY6fvWW
> > > > >
> > > > > Re `isEnableIfAliasTemplate`: Again, the problem is that Clang hard-codes the names of these entities. Look at https://godbolt.org/z/W5Yf6qGM6 — change `E` from `enable_if` to `enable_if2`, change `ET` from `enable_if_t` to `enable_if_t2`, and observe how the diagnostics change their wording in response to the various name-changes. I think this is bad of Clang.
> > > > Concretely, I don't think trying to guess that a class is a `enable_if`-like thing based on its shape is a good idea. It's too complicated and can have false positives. If we were to do something, I'd do something dumb like add `_EnableIf` to the list of things we look for, or something very constrained along those lines.
> > > >
> > > > > Here's why: the alias's expansion is observable by the programmer. https://godbolt.org/z/YeaY6fvWW
> > > >
> > > > Ah, geez, indeed.
> > > >
> > > >
> > > > I think what we need to settle on is this:
> > > >
> > > > > I guess the question I have here is "why do you prefer `_EnableIf` over `enable_if_t`? My mental model is that we strive to use standard utilities when we can, and we use shims like `_EnableIf` (which should really be called `__enable_if_t`) when we need the utility in C++N but it's not standardized until some later C++M version. Just like we have e.g. `__to_address` for pre-C++20 code. I think it would be good to agree on this general question.
> > > >
> > > > Concretely, I don't think trying to guess that a class is a `enable_if`-like thing based on its shape is a good idea. It's too complicated and can have false positives.
> > >
> > > I think it's a great idea and there's no such thing as a false positive (since the diagnostic is in the form of an additional explanatory note that specifically //adds// information about the boolean expression that evaluated to `false` instead of `true`)... but that disagreement may be moot, since I don't think anyone's stepping up to actually write that improved diagnostic anytime soon.
> > >
> > > > I guess the question I have here is "why do you prefer `_EnableIf` over `enable_if_t`? My mental model is that we strive to use standard utilities when we can, and we use shims when we need the utility in C++N but it's not standardized until some later version.
> > >
> > > I tend to see everything in terms of "maintainability bottlenecks" (there must be a standard term for this, but I'm sticking with "bottleneck" for the purposes of this message). Whenever we use a third-party dependency — whether it's Boost or PCRE or std::regex or std::enable_if_t — we have a choice of using it //directly// in our codebase (scattering its uses all over), or else piping it through a deliberately constructed bottleneck where we can easily swap it out for something else. For some things, like `std::regex`, we obviously want to bottleneck it, because the dependency sucks so badly that we //know// we'll want to replace it later. For some things like `std::string`, we obviously //don't// want to bottleneck it. In an average industry codebase, we don't care to bottleneck `std::enable_if_t` either, because it's not a hotspot; but for libc++, where it's used super heavily, and where we //know// a better replacement, I think it's reasonable to bottleneck it the way Eric did.
> > >
> > > You're right that sometimes we bottleneck things because we need to conditionally replace them with a polyfill (like how codebases will have a namespace alias `fs` which is either `std::filesystem` or `boost::filesystem`; and I recently did the same thing with `namespace asio`) but polyfilling is not the //only// reason we've ever bottlenecked something.
> > I understand exactly what you mean (the term I use in my head when I think about those is "indirection points", but I guess everyone has their personal way of thinking about it).
> >
> > Concretely, do we have a suggestion where we keep using `_EnableIf` but also get better diagnostics than the terrible ones we get today? If not, I think this patch is pretty important. Also, I'd be curious to see what is the actual benefit of using `_EnableIf` the way we have it today -- it's not clear to me the compile-time cost comes from instantiating `enable_if<cond, void>` more than it comes from evaluating any expression that forms the `cond`.
> > Concretely, do we have a suggestion where we keep using _EnableIf but also get better diagnostics than the terrible ones we get today? If not, I think this patch is pretty important.
>
> I just tried my suggestion of aliasing `_EnableIf` to `enable_if_t<_Bp, _Tp>`. It turns out that Clang's diagnostic is already not even //that// smart: it loses the spelling of the boolean condition.
> https://godbolt.org/z/eWoe9M986
> ```
> <source>:13:36: note: candidate template ignored: requirement 'false' was not satisfied [with _Fn = Evil &]
> _LIBCPP_CONSTEXPR_AFTER_CXX17 auto not_fn1(_Fn&& __f) {
> ^
> ```
> (That Godbolt has `std::not_fn` with the current technique, `std::not_fn1` with the aliasing technique, and `std::not_fn2` with your proposed direct use of `enable_if_t`.)
>
> I think it'd be worth pursuing the Clang patch ("We failed to find a member in a class template of the form `template<[...], bool, [...]>`, where that `bool` parameter is `false`, and where, if the `bool` parameter had been `true`, we //would// have found such a member"), but concretely, no, I'm not planning to pursue it myself.
>
> > Also, I'd be curious to see what is the actual benefit
>
> So would I. Do you think it would be useful to run (some part of) the test suite (in compile-only mode, if such a thing exists) in both configurations, and see if there's any detectable difference in how long it takes? I tried just now with HyperRogue, but of course there was no detectable difference. To see a difference you'd have to be doing a lot of instantiations of enable-iffed libc++ components, which is already a difficult achievement.
>
> Then I went further into the rabbit hole with this Python script to generate .cpp files to test:
> ```
> print("""
> #include <type_traits>
> template<int N> struct priority_tag : priority_tag<N-1> {};
> template<> struct priority_tag<0> {};
> template<int N> struct A {};
> template<bool> struct MetaBase {};
> template<> struct MetaBase<true> { template<class T> using If = T; };
> template<bool B, class T> using EnableIf = typename MetaBase<B>::template If<T>;
> """)
>
> # OPTION NUMBER ONE
> for i in range(200):
> print("template<int N> auto f(priority_tag<%d>) -> std::enable_if_t<N==%d, A<%d>>;" % (i, i, i))
>
> # OPTION NUMBER TWO
> #for i in range(200):
> # print("template<int N> auto f(priority_tag<%d>) -> EnableIf<N==%d, A<%d>>;" % (i, i, i))
>
> # OPTION NUMBER THREE
> #for i in range(200):
> # print("template<int N> auto f(priority_tag<%d>) -> A<%d> requires (N==%d);" % (i, i, i))
>
> print("void test() {")
> for i in range(200):
> print("f<%d>(priority_tag<200>{});" % i)
> print("}")
> ```
> But still this produced no significant results. (~2.5 seconds to compile options 1 and 2; ~3.0 seconds to compile option 3. I'm surprised by that Concepts slowdown, but my numbers are also //very// noisy and might be completely wrong.)
>
> `_EnableIf` seems to have happened rather quickly in 51a741c87fa8f87c8b37015dac4bce2b10d1ce81 (2019-06-21) and then 3359a17b3aef1effa494da3abe7f438f5bb184a7 (2019-06-23) — no Differentials, no links to benchmarks in the commit messages. Maybe it's //not// justified on performance grounds. @EricWF, any thoughts?
I applied this patch so that we only compile everything in the test suite:
```
diff --git a/libcxx/utils/libcxx/test/format.py b/libcxx/utils/libcxx/test/format.py
index e4e2937f0a5a..064db5627677 100644
--- a/libcxx/utils/libcxx/test/format.py
+++ b/libcxx/utils/libcxx/test/format.py
@@ -249,7 +249,6 @@ class CxxStandardLibraryTest(lit.formats.TestFormat):
elif filename.endswith('.link.fail.cpp'):
steps = [
"%dbg(COMPILED WITH) %{cxx} %s %{flags} %{compile_flags} -c -o %t.o",
- "%dbg(LINKED WITH) ! %{cxx} %t.o %{flags} %{link_flags} -o %t.exe"
]
return self._executeShTest(test, litConfig, steps)
elif filename.endswith('.verify.cpp'):
@@ -267,7 +266,6 @@ class CxxStandardLibraryTest(lit.formats.TestFormat):
elif filename.endswith('.pass.cpp') or filename.endswith('.pass.mm'):
steps = [
"%dbg(COMPILED WITH) %{cxx} %s %{flags} %{compile_flags} %{link_flags} -o %t.exe",
- "%dbg(EXECUTED AS) %{exec} %t.exe"
]
return self._executeShTest(test, litConfig, steps)
# This is like a .verify.cpp test when clang-verify is supported,
```
This is what I got:
With enable_if_t (my patch)
---------------------------
```
ninja -C build/default check-cxx 5509.93s user 987.85s system 1737% cpu 6:13.89 total
ninja -C build/default check-cxx 5513.07s user 991.30s system 1866% cpu 5:48.45 total
```
With _EnableIf (status quo)
---------------------------
```
ninja -C build/default check-cxx 5508.53s user 1069.88s system 1876% cpu 5:50.60 total
ninja -C build/default check-cxx 5514.48s user 1000.38s system 1874% cpu 5:47.62 total
```
>From the test suite at least, I'm not seeing any significant difference. That may not be the best benchmark though.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D108216/new/
https://reviews.llvm.org/D108216
More information about the libcxx-commits
mailing list