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
13 ↗(On Diff #218275)

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
1104

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
1268

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
1104

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
13 ↗(On Diff #218275)

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.Sep 19 2019, 2:50 PM
zoecarver added inline comments.
clang/include/clang/Parse/Parser.h
2719

These methods are generalized more in D67588.

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

This should say ref in the name.

clang/lib/Parse/ParseDeclCXX.cpp
1103

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.Sep 19 2019, 3:07 PM
clang/lib/Parse/ParseDeclCXX.cpp
1108

I think this should be generalized and merged with ParseUnderlyingTypeSpecifier.

Maybe called ParseTransformationTypeSpecifier.

clang/lib/Sema/SemaType.cpp
1642

This should be merged with the above case.

5689

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

clang/test/SemaCXX/add_reference.cpp
66 ↗(On Diff #218276)

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.Sep 19 2019, 6:53 PM
zoecarver added inline comments.
clang/lib/Sema/SemaType.cpp
1642

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.Sep 20 2019, 6:24 AM
  • address review comments
  • fix warnings in build
zoecarver marked an inline comment as done.Sep 20 2019, 6:27 AM
zoecarver added inline comments.
clang/lib/AST/ItaniumMangle.cpp
3500

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

Friendly ping. Other than type mangling, does anything need to be fixed?

zoecarver updated this revision to Diff 243834.Feb 11 2020, 5:38 AM
  • Combine D67052 and D67588.
  • Remove __add* type traits.
  • Update mangling/dumping/encoding of all traits.
  • Remove stress tests.
  • Address comments in D67588.

I'll work on some failing tests and add those when they're ready.

zoecarver marked 3 inline comments as done.Feb 11 2020, 5:51 AM
zoecarver added inline comments.
clang/lib/AST/TypePrinter.cpp
1024

These need the opening paren.

1040

Missing RemoveReference.

clang/lib/Parse/ParseDeclCXX.cpp
1111

I'll work on your comment:

It would be really cool if we could simplify these conversions to static_cast<DeclSpec::TST>(Tok.getKind() - base);

Some nits, looks great, especially for library concepts. Pinging @cjdb

clang/lib/AST/TypePrinter.cpp
1022–1023

Couldn't we use TextNodeDumper::VisitUnaryTransformType to dump the string and then simplify it to a single case.

Given the possibility that maybe some day one might add the add_foo builtins

clang/lib/Parse/ParseDeclCXX.cpp
1116

I dont think that this message is valid anymore now that you also remove qualifiers.

EricWF added a comment.Mar 1 2020, 2:23 PM

LGTM once all of the inline comments are addressed.

After that, this is ready to land.

clang/lib/AST/ItaniumMangle.cpp
3500

@rsmith Can you double check the mangling changes?

clang/lib/Sema/SemaType.cpp
1625

Why can't we share the implementation with TST_underlyingType?

miscco added inline comments.Apr 17 2020, 6:39 AM
clang/include/clang/Sema/DeclSpec.h
416

There is a bug here, this should probably be

return (TST_typename <= T && T <= TST_atomic);
zoecarver marked 6 inline comments as done.Apr 20 2020, 1:43 PM

@EricWF and @miscco thanks for the review comments. Sorry for the delay, I forgot about this patch. All your comments have been addressed/fixed.

clang/lib/AST/TypePrinter.cpp
1022–1023

Couldn't we use TextNodeDumper::VisitUnaryTransformType to dump the string and then simplify it to a single case.

Good call. Will do.

Given the possibility that maybe some day one might add the add_foo builtins

I had a patch that added those builtins but, we decided that there was no real reason for them to exist.

clang/lib/Sema/SemaType.cpp
1625

Yep, fixed.

zoecarver updated this revision to Diff 258834.Apr 20 2020, 1:45 PM
zoecarver marked 2 inline comments as done.
  • Rebase
  • Fix based on review comments
zoecarver updated this revision to Diff 258836.Apr 20 2020, 1:48 PM
  • Format using clang-format
Harbormaster completed remote builds in B54003: Diff 258836.