Page MenuHomePhabricator

Add reference type transformation builtins
Needs ReviewPublic

Authored by zoecarver on Sep 1 2019, 2:33 PM.

Details

Summary

This patch adds builtin type traits to transform reference types. Specifically, it adds __add_lvalue_reference, __add_rvalue_reference, and __remove_reference. The first two builtins speed up builds by around 3x while the last builtin only sees small improvements (we may be able to optimize it more, though). Once added to the standard library, this should make libc++ (and other code) much faster to compile.

I tried to generalize as much of the builtin as possible so, the only functional difference between the three builtins is in the file BuildUnaryTransformType.

Event Timeline

zoecarver created this revision.Sep 1 2019, 2:33 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 1 2019, 2:33 PM
zoecarver marked an inline comment as done.Sep 1 2019, 2:37 PM
zoecarver added inline comments.
libcxx/test/libcxx/utilities/meta/stress_tests/stress_test_remove_reference.sh.cpp
14

I think there is a good chance the reason that my added builtin isn't significantly faster is that std::remove_reference is already optimized somewhere. Thoughts?

zoecarver updated this revision to Diff 218276.Sep 1 2019, 2:38 PM
  • remove accedentally added file
Mordante added inline comments.
clang/lib/Parse/ParseDeclCXX.cpp
1095

Wouldn't it be better to use llvm_unreachable("Not a reference type specifier");
Maybe also move this line out of the switch, allowing the compiler to warn about not handled enumeration values.

clang/lib/Sema/SemaType.cpp
1266

Same here?

If you're going to do __add__lvalue_reference, __add_rvalue_reference, and __remove_reference, why not go all the way and add __is_reference, __is_lvalue_reference and __is_rvalue_reference?

If you're going to do __add__lvalue_reference, __add_rvalue_reference, and __remove_reference, why not go all the way and add __is_reference, __is_lvalue_reference and __is_rvalue_reference?

Those already exist :)

I am planning on adding others, __remove_cv, __decay, etc. but wanted to land this first and make sure I was doing it right.

I'll update type_traits in libc++ after this lands. I'm also planning on making a patch to update a few of the builtin type traits. That should significantly speed up build times. FYI here is all the "type trait primitives" clang supports. At some point, we should try to support all of them.

zoecarver marked an inline comment as done.Sep 3 2019, 6:06 PM
zoecarver added inline comments.
clang/lib/Parse/ParseDeclCXX.cpp
1095

I'll update it to use llvm_unreachable but, I don't think we want the compiler to warn about unhandled enumeration values.

zoecarver marked an inline comment as done.Sep 3 2019, 6:12 PM
zoecarver added inline comments.
libcxx/test/libcxx/utilities/meta/stress_tests/stress_test_remove_reference.sh.cpp
14

I think if I updated this test to actually remove a reference, it might show more of a difference.

zoecarver marked an inline comment as done.Thu, Sep 19, 2:50 PM
zoecarver added inline comments.
clang/include/clang/Parse/Parser.h
2606

These methods are generalized more in D67588.

EricWF added inline comments.Thu, Sep 19, 2:50 PM
clang/include/clang/AST/Type.h
4368

This should say ref in the name.

clang/lib/Parse/ParseDeclCXX.cpp
1094

Don't use default switch cases. It suppresses non-covered switch warnings, which we want fire when a new enumerator is added.

EricWF added inline comments.Thu, Sep 19, 3:07 PM
clang/lib/Parse/ParseDeclCXX.cpp
1099

I think this should be generalized and merged with ParseUnderlyingTypeSpecifier.

Maybe called ParseTransformationTypeSpecifier.

clang/lib/Sema/SemaType.cpp
1612

This should be merged with the above case.

5496

DS.getTypeSpecType() >= DeclSpec::TST_underlyingType && DS.getTypeSpecType() <= TST_removeReferenceType would correct this assertion.

clang/test/SemaCXX/add_reference.cpp
67

You should test a lot more types here. reference types, const types, pointer types, function types, non-referenceable types.

zoecarver marked 5 inline comments as done.Thu, Sep 19, 6:53 PM
zoecarver added inline comments.
clang/lib/Sema/SemaType.cpp
1612

The reason I haven't done that is because __underlying_type will default to IntTy if it fails. We don't want that for the others (maybe we don't want that here either?).

zoecarver updated this revision to Diff 221014.Fri, Sep 20, 6:24 AM
  • address review comments
  • fix warnings in build
zoecarver marked an inline comment as done.Fri, Sep 20, 6:27 AM
zoecarver added inline comments.
clang/lib/AST/ItaniumMangle.cpp
3412

Eric, I looked at your comment, but it seems that we no longer unary type transformations that way. @rsmith is this correct?