Page MenuHomePhabricator

Add builtin trait for add/remove cv (and similar)
AbandonedPublic

Authored by zoecarver on Sep 14 2019, 10:42 AM.

Details

Summary

This patch adds six builtins: __remove_cv, __remove_cosnt, __remove_volatile, __add_cv, __add_const, and __add_volatile. I have added two stress tests to show the performace improvements. The __remove_cv test sees a 160% build time imporvement while the __add_cv test sees an 8% improvement.

This patch is based on D67052.

I will submit a patch for implementing these in libc++ after this lands.

Diff Detail

Event Timeline

zoecarver created this revision.
  • diff from D67052, not master
Harbormaster completed remote builds in B38138: Diff 220226.

Generate diff based on D67052 (arc wasn't working so I had to do it manually this time).

The __remove_cv test sees a 160% build time imporvement while the __add_cv test sees an 8% improvement.

Those numbers are specifically for those macrobenchmarks, right?
What impact does this have on some real-world code?
I'm just curious as to cost/benefit here.

@lebedev.ri after adding _only_ these builtins to libc++ the type trait tests run several seconds faster. I think if we update _all_ the type traits to use builtins then, it could increase speed of the type trait tests by as much as 50% (if not more).

LGTM other than the inline comments.
I'll take a second pass at this once they're addressed.

Let's land the patch this week!

clang/include/clang/Sema/DeclSpec.h
419

Is the block of values for these identifiers contiguous? Can we work towards writing this as min <= x <= max?

clang/lib/Parse/ParseDeclCXX.cpp
1094

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

That said, it's probably easier to land this patch as is.

clang/lib/Sema/SemaType.cpp
8475

I don't know that we need add_foo, because if I want to optimize my type-trait, I'll just write T const.

Adding code to clang has a maintenance cost; and that's non-trivial. The remove_foo traits add a lot of value, and that justifies the cost.

clang/test/SemaCXX/add_cv.cpp
43

Clang tests normally test:

(A) The diagnostics that emit when people seriously misuse.
(B) Just some failures.

libcxx/test/libcxx/utilities/meta/stress_tests/stress_test_add_cv.sh.cpp
2

I think we probably want to keep the libc++ changes in a separate patch.

zoecarver abandoned this revision.Feb 11 2020, 5:41 AM

Closing in favor of D67052.