Page MenuHomePhabricator

[libc++] Use builtin type traits whenever possible
ClosedPublic

Authored by zoecarver on Sep 22 2019, 8:33 PM.

Details

Summary

This patch updates <type_traits> to use builtins wherever possible. This patch should increase performance greatly. When I compiled all the type traits tests before and after, I saw a 6.6% decrease in build time.

There are a few traits who's corresponding builtins do not work properly. See the comments in code. We will have to enable these traits only after a particular version of clang where the issue has been fixed.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Like Richard said before, we can't switch type traits to aliases. However, we can always do:

template <class _Tp>
struct is_const : integral_constant<bool, __is_const(_Tp)> { };

Can you switch back all the aliases to structs like that?

This revision now requires changes to proceed.Oct 8 2019, 11:06 AM
  • Merge branch 'master' into use-builtin-type-traits
  • fix: overload resolution issue
  • fix: C++03 mode issue
CaseyCarter added inline comments.
libcxx/include/type_traits
565

Duplicate this on both branches of your new conditional, and optimize it on the __has_keyword(__is_same) branch?

572

It's peculiar that this (and line 576) uses #ifdef __clang__ vs. your __has_keyword(__is_same). Should the two be consistent? Should _IsSame and _IsNotSame move up into your new conditional?

691

Ditto "Should the variable template form be optimized as well to avoid the class template instantiation?" (I'll stop marking these, but IMO all of the is_meow_v variable templates should use the intrinsics directly rather than unnecessarily instantiate a class template.)

793

This isn't equivalent: __is_void(cv-void) is true, but __libcpp_is_void<cv-void> is false.

952
  1. Why are these grouped like this, when none of the above are so grouped?
  2. Shouldn't is_rvalue_reference and is_reference have leading double underscores?
1222

Should this be >= 1000 instead of > 900 to avoid catching e.g. 9.0.1, or has the bug been fixed on the Clang 9 branch also? (If so, the comment could be clarified.)

1460

C++20 would greatly appreciate a __remove_cvref(T) intrinsic which could be used to implement the above.

1507

Ditto "Should this actually be _LIBCPP_CLANG_VER >= 1000, or should the comment be clarified?"

libcxx/test/std/utilities/meta/meta.rel/is_same.pass.cpp
61

Maybe add a void fn(std::true_type) { } to ensure that std::is_same<T, meow> is not true_type even when T and meow are the same type?

62
  1. std::disjunction is a C++17 feature.
  2. std::disjunction</* ...stuff... */, std::false_type> is a different type than, but has the same base characteristic as std::disjunction</* ...stuff... */>, so the false_type here does nothing.
  3. The body of this member function is not instantiated for any specialization of this class template in this test file, so it is extraneous.
  4. This member does nothing useful, so there seems to be no need to instantiate it.
84

Defining variables suffices to instantiate the class template specializations, so this assert seems to serve no useful purpose.

zoecarver marked 9 inline comments as done.Oct 20 2019, 8:33 PM
zoecarver added inline comments.
libcxx/include/type_traits
565

I am planning on doing this but, I am going to do it in another patch.

572

IIRC clang has always had __is_same whereas that is not true for all these traits. I wasn't always sure (and I didn't want to write a lot of "random" version #ifs) so, to be safe, I looked for the keyword. It might be good to check for __clang__ as well, though.

691

Agreed. See my comment above.

793

I'll look into this and submit a patch, thanks.

952
  1. I assumed that if one of them was defined then, they all would be defined.
  2. Yep, I'll fix it.
1222

When clang 9.0.1 branched? I submitted it a few weeks ago. I'll see if I can figure out what version this is in an update the condition/comment.

1460

See D67052 and D67588 :)

libcxx/test/std/utilities/meta/meta.rel/is_same.pass.cpp
62

Thanks for the notes. I wrote this test a bit hastily. I'll fix it up.

84

It serves the purpose of unused variable compiler errors.

zoecarver marked 2 inline comments as done.Oct 20 2019, 8:40 PM
zoecarver added inline comments.
libcxx/include/type_traits
793

I see the issue, std::is_void functions the same way. I'll update the patch and pull __libcpp_is_void out of the conditional block.

1222

It doesn't look like clang 9.0.1 has been released yet so the fix will be in it. I'll update the comment.

CaseyCarter marked an inline comment as done.Oct 21 2019, 9:09 AM
CaseyCarter added inline comments.
libcxx/include/type_traits
572

Some Compiler Explorer research (https://godbolt.org/z/u1JVsC) shows that __is_same is supported at least back to clang 3.0.0, but __is_identifier (which is used to implement __has_keyword) was implemented in 3.5. Consequently there's a range of versions that will use __is_same here but not above, which is weird. It seems to me that line 547 should simply check #ifdef __clang__ since the library does not support clang versions that don't have __is_same (if any exist).

1460

I suppose the extra few nanoseconds that __remove_cv(__remove_reference(T)) costs over a __remove_cvref(T) is negligible. It's certainly in the noise compared to this alias that instantiates a class that instantiates another class that instantiates *two* classes. ;)

libcxx/test/std/utilities/meta/meta.rel/is_same.pass.cpp
84

I believe it's conventional in the test suite to silence unused variable warnings by casting to void, e.g., (void) t1;. In this particular case, however, you don't need the names for anything: OverloadTest only needs to be instantiated to achieve its purpose. You could simply create temporaries instead of named variables a la OverloadTest<int>{}; OverloadTest<char>{}; which avoids defining any variables in the first place.

zoecarver updated this revision to Diff 226196.EditedOct 23 2019, 6:36 PM
zoecarver marked 2 inline comments as done.
  • fix the std::is_same test
zoecarver updated this revision to Diff 226197.Oct 23 2019, 6:43 PM
zoecarver marked 6 inline comments as done.
  • update based on review
libcxx/include/type_traits
572

Clang 3.5.0 is the latest supported version so, that should be fine.

libcxx/test/std/utilities/meta/meta.rel/is_same.pass.cpp
84

I can't use braces because of C++03 mode. I fixed it by casting to void, thanks for the suggestion.

Ping @ldionne any other comments?

Ping. I'd like to get this approved before I rebase off master again. Any other review comments?

cc @ldionne @EricWF

ldionne requested changes to this revision.Wed, Mar 11, 12:16 PM

Thanks for your patience! This is looking good, but I do have some comments.

libcxx/include/type_traits
564

I think it's reasonable to change the meow_v variable templates to use intrinsics in this patch too, WDYT? Especially since you're doing it for is_integral_v below.

676

_LIBCPP_TEMPLATE_VIS (here and elsewhere)

793

I would remove that altogether since it's not used.

799

We could use this instead:

template <class _Tp> struct _LIBCPP_TEMPLATE_VIS is_void : is_same<typename remove_cv<_Tp::type, void> {};

Arguably, only old compilers won't use the intrinsic, so I think this simplification (especially the removal of __libcpp_is_void) is worth it.

This revision now requires changes to proceed.Wed, Mar 11, 12:16 PM
zoecarver marked an inline comment as done.Sat, Mar 14, 1:56 PM
zoecarver added inline comments.
libcxx/include/type_traits
564

Yep. Will do.

Herald added a reviewer: Restricted Project. · View Herald TranscriptSat, Mar 14, 1:56 PM
zoecarver updated this revision to Diff 250383.Sat, Mar 14, 2:04 PM
  • Rebase off master
  • Add _LIBCPP_TEMPLATE_VIS everwhere
  • Add *_v everywhere
  • Remove add_reference and friends
zoecarver updated this revision to Diff 250385.Sat, Mar 14, 2:16 PM
  • Support underlying_type and is_enum
ldionne accepted this revision.Wed, Mar 18, 3:19 PM

LGTM if the is_enum test is sufficient.

libcxx/include/type_traits
1161

__is_enum is not guarded by __has_keyword like the other ones?

This revision is now accepted and ready to land.Wed, Mar 18, 3:19 PM
zoecarver marked an inline comment as done.Thu, Mar 19, 8:59 AM

I'll look into the CI failure.

libcxx/include/type_traits
1161

__is_enum (and __underlying_type) are already being used in the implementation. IIRC early versions of clang only supported __has_keyword. But I'm pretty sure we don't support any versions of clang that don't have both.

zoecarver updated this revision to Diff 251401.Thu, Mar 19, 9:06 AM
  • Remove changes to __underlying_type (I didn't see the is_enum check.)

The CI just saved me at least half an hour of work (likely more) and three commits :)

I'll make sure the tests pass on the CI, then I'll commit.

This revision was automatically updated to reflect the committed changes.

74494d9992bdf266259f8add410f346971471c46 fixed both the issue with is_fundamental and is_compound (which failed because it's implemented using is_fundamental).

9e2207a00bddf9cf5f3f58b79dd63d01bb898198 adds the missing closing bracket in the is_void non-builtin implementation. If there's another failure I'll revert all three commits. It's extremely hard to test all paths of this patch because almost every compiler has a different set of the builtins.

tcwang added a subscriber: tcwang.Tue, Mar 31, 2:29 PM

This patch has caused a regression on Chrome OS (https://bugs.chromium.org/p/chromium/issues/detail?id=1065276).

Some errors like:
In file included from ../../../libchrome-576279/platform2/libchrome/mojo/core/ports/event.cc:5:
libchrome-576279-r9: In file included from ../../../libchrome-576279/platform2/libchrome/mojo/core/ports/event.h:10:
libchrome-576279-r9: In file included from /usr/bin/../include/c++/v1/vector:274:
libchrome-576279-r9: In file included from /usr/bin/../include/c++/v1/__bit_reference:14:
libchrome-576279-r9: In file included from /usr/bin/../include/c++/v1/bit:57:
libchrome-576279-r9: /usr/bin/../include/c++/v1/limits:190:59: error: invalid operands to binary expression ('float128' and 'int')
libchrome-576279-r9: static _LIBCPP_CONSTEXPR const _Tp value = _Tp(_Tp(1) <<
digits);
libchrome-576279-r9: ~~~~~~ ^ ~~~~~~~~
libchrome-576279-r9: /usr/bin/../include/c++/v1/limits:211:49: note: in instantiation of template class 'std::1::libcpp_compute_min<float128, 127, true>' requested here
libchrome-576279-r9: static _LIBCPP_CONSTEXPR const type
min = libcpp_compute_min<type, digits, is_signed>::value;
libchrome-576279-r9: ^
libchrome-576279-r9: /usr/bin/../include/c++/v1/limits:444:15: note: in instantiation of template class 'std::
1::libcpp_numeric_limits<float128, true>' requested here
libchrome-576279-r9: : private libcpp_numeric_limits<typename remove_cv<_Tp>::type>
libchrome-576279-r9: ^
libchrome-576279-r9: ../../../libchrome-576279/platform2/libchrome/base/numerics/safe_conversions_impl.h:29:39: note: in instantiation of template class 'std::
1::numeric_limits<float128>' requested here
libchrome-576279-r9: ? std::numeric_limits<NumericType>::max_exponent
libchrome-576279-r9: ^
libchrome-576279-r9: ../../../libchrome-576279/platform2/libchrome/base/numerics/safe_conversions_impl.h:175:7: note: in instantiation of template class 'base::internal::MaxExponent<
float128>' requested here
libchrome-576279-r9: MaxExponent<Dst>::value > MaxExponent<Src>::value
libchrome-576279-r9: ^
libchrome-576279-r9: ../../../libchrome-576279/platform2/libchrome/base/numerics/safe_conversions.h:240:29: note: in instantiation of template class 'base::internal::StaticDstRangeRelationToSrcRange<float128, unsigned long, base::internal::INTEGER_REPRESENTATION_SIGNED, base::internal::INTEGER_REPRESENTATION_UNSIGNED>' requested here
libchrome-576279-r9: static const bool value = StaticDstRangeRelationToSrcRange<Dst, Src>::value ==
libchrome-576279-r9: ^
libchrome-576279-r9: ../../../libchrome-576279/platform2/libchrome/base/numerics/safe_conversions.h:286:17: note: in instantiation of template class 'base::internal::IsNumericRangeContained<
float128, unsigned long, void>' requested here
libchrome-576279-r9: IsNumericRangeContained<Dst, T>::value>::type* = nullptr>
libchrome-576279-r9: ^
libchrome-576279-r9: ../../../libchrome-576279/platform2/libchrome/base/numerics/safe_conversions.h:287:13: note: while substituting prior template arguments into non-type template parameter [with Dst = float128]
libchrome-576279-r9: constexpr operator Dst() const {
libchrome-576279-r9: ^~~~~~~~~~~~~~~~~~~~~~
libchrome-576279-r9: ../../../libchrome-576279/platform2/libchrome/mojo/core/ports/event.cc:171:55: note: while substituting deduced template arguments into function template 'operator type-parameter-0-0' [with Dst =
float128, $1 = (no value)]
libchrome-576279-r9: if (!total_size.IsValid() || num_bytes < total_size.ValueOrDie())

@tcwang I'll try to put together a fix later today. My guess is that one of these type trait builtins doesn't play nice with __float128. But, that's only a guess.

Also, in the future, it's easier to read the error if you use triple backticks. That way the formatting/indentation isn't messed up.

event.ii is the preprocessed test case that can be used to reproduce the error. I am trying to use c-reduce to reduce but due to many errors (25 in total), the reduced test case is actually having other errors. I will try to reduce it in the meantime. But just in case this can help the author to figure out the problem. Also attached the interestingness test in test.sh{F11652738}

@tcwang here's you're reproducer: https://godbolt.org/z/RJtTBG

Looks like we don't have any __float128 tests not only in numeric limits but in libc++ altogether.

Anyway, to address the problem: is_arithmetic_v<__float128> is true after this patch and false before this patch. It seems like returning true is the correct behavior so the change should probably happen in numeric limits. It's not entirely clear what that change should be, though. Do we want to support __float128 in numeric_limits? We don't seem to support it anywhere else. Maybe add a static_assert so that future bugs are easier to track down?

In the meantime, numeric_limits just returned 0 for numeric_limits< __float128 >::min() so maybe you can replace the current use with that?

I'm going to plan to revert the is_arithmetic and is_floating_point changes later today. Any objections?

Copying an email reply to phab thread from @EricWF , not sure why it did not make it here.

We should revert the change for is_arithmetic and is_floating_point.

Marshall and I have discussed the __float128 issue previously, but we couldn't reach an agreement that it should be handled as an extension.
For that reason, I think we should go back to the old behavior.

@tcwang here's you're reproducer: https://godbolt.org/z/RJtTBG

Thank you!

Looks like we don't have any __float128 tests not only in numeric limits but in libc++ altogether.

Anyway, to address the problem: is_arithmetic_v<__float128> is true after this patch and false before this patch. It seems like returning true is the correct behavior so the change should probably happen in numeric limits.

Thanks again for figuring out the problem.

It's not entirely clear what that change should be, though. Do we want to support __float128 in numeric_limits? We don't seem to support it anywhere else. Maybe add a static_assert so that future bugs are easier to track down?

In the meantime, numeric_limits just returned 0 for numeric_limits< __float128 >::min() so maybe you can replace the current use with that?

I don't think I understand what to replace. From the log I showed before, the error was from the header file vector all the way to /usr/bin/../include/c++/v1/limits. Plus we don't own the code that has the error, and I didn't see float128 directly in the code. And I see many places using ::max() (not float128 type, or not clear to me it's float128 type). So I don't know how to proceed with it...

@tcanens I didn't look closely enough at the error. It looks like it's because vector uses numeric limits. I'll revert the changes which will fix the problem so, don't worry about it :)

tcanens removed a subscriber: tcanens.Wed, Apr 1, 12:37 PM

@tcanens I didn't look closely enough at the error. It looks like it's because vector uses numeric limits. I'll revert the changes which will fix the problem so, don't worry about it :)

I'm guessing that you tagged the wrong person?

@tcanens sorry about that. Yes, I meant @tcwang.