This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use enable_if_t instead of _EnableIf
ClosedPublic

Authored by ldionne on Aug 17 2021, 9:41 AM.

Details

Summary

I just ran into a compiler error involving __bind_back and some overloads
that were being disabled with _EnableIf. I noticed that the error message
was quite bad and did not mention the reason for the overload being
excluded. Specifically, the error looked like this:

candidate template ignored: substitution failure [with _Args =
<ContiguousView>]: no member named '_EnableIfImpl' in 'std::_MetaBase<false>'

Instead, when using enable_if or enable_if_t, the compiler is clever and
can produce better diagnostics, like so:

candidate template ignored: requirement 'is_invocable_v<
     std::__bind_back_op<1, std::integer_sequence<unsigned long, 0>>,
     std::ranges::views::__transform::__fn &, std::tuple<PlusOne> &,
     ContiguousView>' was not satisfied [with _Args = <ContiguousView>]

Basically, it tries to do a poor man's implementation of concepts, which
is already a lot better than simply complaining about substitution failure.

Hence, this commit uses enable_if_t instead of _EnableIf whenever
possible. That is both more straightforward than using the internal
helper, and also leads to better error messages in those cases.

I understand the motivation for _EnableIf's implementation was to improve
compile-time performance, however I believe striving to improve error
messages is even more important for our QOI, hence this patch. Furthermore,
it is unclear that _EnableIf actually improved compile-time performance
in any noticeable way (see discussion in the review for details).

Diff Detail

Event Timeline

ldionne requested review of this revision.Aug 17 2021, 9:41 AM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2021, 9:41 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Aug 17 2021, 12:35 PM
Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.
libcxx/include/__functional/bind_front.h
41

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

This revision now requires changes to proceed.Aug 17 2021, 12:35 PM
ldionne added inline comments.Aug 18 2021, 6:50 AM
libcxx/include/__functional/bind_front.h
41

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

libcxx/include/__functional/bind_front.h
41

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

ldionne added inline comments.Aug 23 2021, 11:25 AM
libcxx/include/__functional/bind_front.h
41

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.

libcxx/include/__functional/bind_front.h
41

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.

libcxx/include/type_traits
461

Btw, if you're keeping a version of _EnableIf anyway (for C++11, right?), then I think it's a bad idea to pessimize it. Might as well leave the faster version in place; it's only 4 lines for the fast one versus 2 lines for the slow one.

ldionne added inline comments.Aug 30 2021, 1:31 PM
libcxx/include/__functional/bind_front.h
41

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.

Quuxplusone added inline comments.
libcxx/include/__functional/bind_front.h
41

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?

ldionne added inline comments.Aug 31 2021, 8:26 AM
libcxx/include/__functional/bind_front.h
41

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.

libcxx/include/__functional/bind_front.h
41

I've filed https://bugs.llvm.org/show_bug.cgi?id=51696 for the Clang diagnostic, not that I expect any movement on it anytime soon.

ldionne accepted this revision.Sep 1 2021, 7:54 AM

So, on the one hand, I see a noticeable improvement on diagnostics.

On the other hand, I see a potential for a slight compile-time degradation which, while I 100% agree with in theory, I don't have a clear sense of how bad it is.

I think I can argue that with the current evidence, this patch should land, so I'm going to do that unless someone has additional concerns. I'm leaving this patch open for 24hrs to get comments from folks (ping @EricWF in particular), then I'm landing this.

Quuxplusone requested changes to this revision.Sep 1 2021, 12:26 PM

@ldionne wrote:

I think I can argue that with the current evidence, this patch should land, so I'm going to do that unless someone has additional concerns. I'm leaving this patch open for 24hrs to get comments from folks (ping @EricWF in particular), then I'm landing this.

For the record, I'm still opposed to the massive amount of code churn here. Suppose I could get a change into Clang trunk that would "look through" arbitrary AliasTemplates before deciding whether something counted as enable_if_t, so that we would get a good diagnostic from

template<bool B, class T = void> using _EnableIf = typename enable_if<B, T>::type;

...then would you consider leaving all the _EnableIf usages alone? You could still rip out the _MetaBase<B>::_EnableIfImpl<T> backend; I do share your skepticism about the performance impact (and wish @EricWF would weigh in; shouldn't we give more than 72h to reply after pinging him for the first time on this PR?). As I said before:

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

Not only would this preserve the "bottleneck," it would also preserve a (useful, question mark?) distinction between "things that are mandated to use literally enable_if" and "things where we just need to do some SFINAE, via whatever unspecified mechanism is considered best this year." Suppose someone in 2025 wanted to replace all the latter SFINAE with requires; will they be able to do it by a search-and-replace on _EnableIf or will they have to carefully audit every use of enable_if_t to see whether it's mandated or not?

libcxx/include/__functional/bind_front.h
41

I've continued down the rabbit hole far enough for a blog post:
https://quuxplusone.github.io/blog/2021/09/04/enable-if-benchmark/
Also relevant to D109212.

ldionne added inline comments.Sep 7 2021, 10:15 AM
libcxx/include/__functional/bind_front.h
41

Awesome, thanks a lot for looking into this!

So, do we agree that the discussion about whether to use _EnableIf or enable_if_t is moot now, and we can go forward with this patch since it makes diagnostics better?

As a follow-up, we could change instances of class = enable_if_t<...> into enable_if_t<...>* = nullptr to get a slight improvement on GCC. WDYT?

libcxx/include/__functional/bind_front.h
41

Well, as per my comment of 2021-09-01, I'd still prefer to bottleneck _EnableIf — and in fact I'd even prefer to change all existing instances of enable_if into _EnableIf, except where enable_if is required by law — to make it clear that we control these implementation details (except where enable_if is required by law).
However, I admit that (given the current state of Clang's diagnostic engine) that would negate the benefit-in-error-messages you're trying to achieve. So we're at an impasse, and I suppose you win by default, if you want enable_if badly enough; I won't be terribly upset, I'll just maybe get to say "I told you so" in five years when we have to touch them all again. ;)
I agree that I see nothing physically wrong with your suggestion to change class = _EnableIf<...> into _EnableIf<...>* = nullptr (or the equivalent with enable_if_t). (Messing with template parameter kinds that way does break ABI, but all of these templates have inline visibility, so we are cool with the break.)

Actually, isn't there still a logistical question: Are there any _EnableIfs in C++03/11 codepaths? You can't replace those with _VSTD::enable_if_t because it doesn't exist until C++14. So what: leave them as _EnableIf? use typename enable_if<...>::type? introduce a new __enable_if_t for portable use (which, cough cough, is just _EnableIf renamed)?

ldionne added inline comments.Sep 7 2021, 6:35 PM
libcxx/include/__functional/bind_front.h
41

(except where enable_if is required by law).

Are there any such places? I'm genuinely asking, cause I don't think I've noticed any. Usually the standard says "constraints" and that means "enable_if or whatever SFINAE method you want", but I don't think I've seen any places where enable_if exactly is required (except in the definition of enable_if_t).

So we're at an impasse, and I suppose you win by default, if you want enable_if badly enough;

As I said, what I want is to improve the terrible state of our diagnostics today (seriously, they suck pretty bad and the introduction of _EnableIf can be seen as a regression in that regard, which I'm trying to fix a couple years too late). I don't want to gratuitously override you, which is why I'm trying to ask whether you agree that we're at a dead end -- I'd be OK with keeping _EnableIf if we could get nice diagnostics, but we can't, so it seems like this patch (pretty much as-is) is the only way forward that gets us decent diagnostics.

Messing with template parameter kinds that way does break ABI, but all of these templates have inline visibility, so we are cool with the break.

Yes, that is my understanding too.

Are there any _EnableIfs in C++03/11 codepaths? You can't replace those with _VSTD::enable_if_t because it doesn't exist until C++14. So what: leave them as _EnableIf?

Yes. The intent of this patch was to only touch code paths that were in a Standard mode where enable_if_t is defined by the library, i.e. only C++14-and-above code paths. The code paths where enable_if_t isn't an option were left to use _EnableIf.

use typename enable_if<...>::type? introduce a new __enable_if_t for portable use (which, cough cough, is just _EnableIf renamed)?

I left those code paths to use _EnableIf to avoid touching them, but I agree that a more consistent approach would be to use __enable_if_t -- and I'd perform such a renaming as a follow-up NFC patch afterwards, I think.

ldionne updated this revision to Diff 371232.Sep 7 2021, 6:41 PM
ldionne retitled this revision from [libc++] Formulate _EnableIf in terms of std::enable_if to [libc++] Use enable_if_t instead of _EnableIf.
ldionne edited the summary of this revision. (Show Details)

Rebase onto main

Quuxplusone added inline comments.Sep 7 2021, 7:28 PM
libcxx/include/__functional/bind_front.h
41

(except where enable_if is required by law).

Are there any such places?

I didn't know either, but I believed not. Grepping libc++ for enable_if could have been my way to find out. ;) In fact I just now grepped the draft source, and surprisingly it mentions enable_if only in the place it's defined (not even in any examples or anything!), so indeed, there are no such places.

I left those code paths to use _EnableIf to avoid touching them, but I agree that a more consistent approach would be to use __enable_if_t

...But if you agree to use something-that's-not-enable_if_t in those pre-'14 places, then why not be consistent and use that pre-'14 thing in all places? And then why not call it _EnableIf, and then we have the status quo back.
The parallel discussion of _LIBCPP_NODISCARD_EXT did make me wonder if we could just do this:

#if _LIBCPP_STD_VER > 11
#define _LIBCPP_ENABLEIF enable_if_t
#else
template<bool _Cp, class _Tp = void> using _EnableIf = typename enable_if<_Cp, _Tp>::type;
#define _LIBCPP_ENABLEIF _EnableIf
#endif

and then search-and-replace to consistently use _LIBCPP_ENABLEIF everywhere. I believe this would give you the nice error messages you want in '14 and later, even in the pre-'14 parts of the library.

Meanwhile, in parallel, I've put up this Clang PR: D109411. This would (1) give you the nice diagnostics for the _EnableIf status quo, i.e., eliminate the search-and-replace aspect of D108216; and also (2) give you the nice diagnostics even pre-'14, for both the status quo and my _LIBCPP_ENABLEIF idea above. However, of course it would only give you this on new versions of Clang; it wouldn't help people with older compilers.

Finally, there's a way to get good diagnostics pre-'14. I think it's hacky, but see what you think:

#if _LIBCPP_STD_VER > 11
#define _LIBCPP_ENABLEIF enable_if_t
#else
namespace __enableif {
    template<bool _Cp, class _Tp = void> using enable_if_t = typename enable_if<_Cp, _Tp>::type;
} // namespace __enableif
#define _LIBCPP_ENABLEIF __enableif::enable_if_t
#endif

This would work even with old versions of Clang, because they check only the name of the alias template, not what namespace it's in.
Of course we could also just say "screw it" and provide std::enable_if_t as a conforming extension in C++11 mode.

I think we should do something that is consistent; I don't like the "half _EnableIf and half enable_if_t" scenario you've got here.
I also think we have agreement that the _MetaBase approach is not buying us any speedup, so I think you could go ahead and ship the three-line refactor of _EnableIf into simply

template<bool _Cp, class _Tp = void> using _EnableIf = typename enable_if<_Cp, _Tp>::type;

if you want to do that, just to get it out of the way, while we continue tussling over the mass-search-and-replace aspect of this patch.

ldionne accepted this revision.Sep 8 2021, 6:09 AM
ldionne added inline comments.
libcxx/include/__functional/bind_front.h
41

If there are no places where enable_if *is* mandated, then I fail to see the point of not using enable_if_t everywhere. The original goal was to be able to tell between places where it is mandated and places where it isn't -- but this distinction doesn't exist.

I don't want to go down the path of using a macro for something as simple as enable_if_t, IMO that's a bad code cleanliness tradeoff to make. I have the same opinion about the namespace idea (although it's a clever idea, indeed). At this point, I'm seeing no reasons not to use enable_if_t in lieu of _EnableIf, so I'm going to move forward with this patch.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 8 2021, 6:09 AM
This revision was automatically updated to reflect the committed changes.