Page MenuHomePhabricator

[clang] adds unary type transformations as compiler built-ins
ClosedPublic

Authored by cjdb on Dec 22 2021, 9:53 PM.

Details

Summary

Adds

  • __add_lvalue_reference
  • __add_pointer
  • __add_rvalue_reference
  • __decay
  • __make_signed
  • __make_unsigned
  • __remove_all_extents
  • __remove_extent
  • __remove_const
  • __remove_volatile
  • __remove_cv
  • __remove_pointer
  • __remove_reference
  • __remove_cvref

These are all compiler built-in equivalents of the unary type traits
found in [[meta.trans]][1]. The compiler already has all of the
information it needs to answer these transformations, so we can skip
needing to make partial specialisations in standard library
implementations (we already do this for a lot of the query traits). This
will hopefully improve compile times, as we won't need use as much
memory in such a base part of the standard library.

[1]: http://wg21.link/meta.trans

Co-authored-by: zoecarver

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
cjdb updated this revision to Diff 450586.Aug 6 2022, 9:37 PM

rebases to ToT

rsmith accepted this revision.Aug 8 2022, 2:52 PM

A handful more comments, but I think we've basically converged here. I'm happy to take another look after you address these if you'd like (or you could ask someone else for a final pass), or for you to land this once you're happy.

Before landing, it'd be good to patch libc++ to use these intrinsics and run its testsuite to look for any unexpected behavior changes, if you've not already done so with this version of the patch.

clang/lib/AST/ASTContext.cpp
10814

I think we don't need this case any more.

clang/lib/Parse/ParseDeclCXX.cpp
1153–1154

This comment still needs to be addressed.

clang/lib/Parse/ParseExpr.cpp
1066–1067

Just curious: why do we only handle six of the traits here, rather than all of them?

clang/lib/Sema/SemaType.cpp
9232–9233

I've been pondering the proper behavior for make_signed on enum E : _BitInt(N) {}. I think the standard wording here is broken (see my email to the lib reflector, subject "make_signed / make_unsigned and padding / extended integer types"), but I'm not certain what the right rule is. Let's say that always producing a _BitInt type in that case is good enough for now. :)

9363

Have you considered moving the calls to getUnaryTransformType into this function? Right now they're scattered among the Builtin* helper functions, and it's hard to be sure that every code path calls it. I think it'd be a lot simpler to create the UnaryTransformType wrapper once, in a single place.

9364

Remove the static -- it's not correct to use a static local for this. There can be multiple Sema instances, and this will pick the types from whichever one was used the first time this function was called.

9366–9370

We don't have an __int128 on 32-bit targets. Maybe below you can form an ArrayRef and slice off the last element if __int128 is unsupported?

9377

Please only compute the size of BaseType once.

9380

Can this fail for an enum whose underlying type is _BitInt(N) for some unusual N?

cjdb updated this revision to Diff 451918.Aug 11 2022, 10:59 AM
cjdb marked 10 inline comments as done.

responds to @richardsmith's feedback

cjdb added a comment.Aug 11 2022, 11:02 AM

Thanks for patiently reviewing! I'll do the libc++ stuff and a pilot run in some code. Hopefully can get it merged tomorrow :-)

clang/lib/Parse/ParseDeclCXX.cpp
1153–1154

Could've sworn I did this one, but it might've gotten mixed up in the D116280 switcheroo.

clang/lib/Parse/ParseExpr.cpp
1066–1067

Why indeed. Perhaps these were the original six I worked on and didn't come back to update this.

1749

You haven't countered this concern, so I'm going to close for now and will happily revisit if this one just slipped by unnoticed.

clang/lib/Sema/SemaType.cpp
9380

Nice catch. This wasn't supposed to happen, but I ended up changing the logic so that _BitInt is considered here instead.

cjdb updated this revision to Diff 452001.Aug 11 2022, 3:22 PM

tl;dr fixes corner-case bug uncovered by libc++

I'd accidentally inverted the logic for when __int128 and unsigned __int128 would be available for underlying enum types. This led to the types being swapped (??) and there wasn't a test to catch it.

Pre-commit CI found build errors that should be addressed.

clang/include/clang/AST/TransformTypeTraits.def
1–2 ↗(On Diff #452001)

Have to fix the formatting manually though.

clang/include/clang/Sema/DeclSpec.h
424–431

Errr, this isn't Python, so that [-1] terrifies me. It took me a good solid ten minutes to convince myself this was actually valid. Any interest in something along the lines of this form?

clang/lib/AST/TypePrinter.cpp
1156–1157

I believe you can.

clang/lib/Parse/ParseDecl.cpp
3476
clang/lib/Sema/SemaType.cpp
9267
9305–9308
9350–9352

An interesting test case for you to consider (both for enumeration underlying types as well as types in general): signed _BitInt requires at least two bits, so trying to do a make_signed on _BitInt(1) should be diagnosed.

cjdb updated this revision to Diff 452259.Aug 12 2022, 11:46 AM
cjdb marked 5 inline comments as done.

applies Aaron's feedback

cjdb marked an inline comment as done.Aug 12 2022, 12:04 PM
cjdb added inline comments.
clang/include/clang/Sema/DeclSpec.h
424–431

I have many questions about the decisions I made in this snippet.

clang/lib/AST/TypePrinter.cpp
1156–1157

It looks like other things also have this as just empty, so maybe it's best to keep it?

aaron.ballman accepted this revision.Aug 12 2022, 12:16 PM

LGTM, though I did have a nit about diagnostic wording that you can feel free to take or leave at your discretion. Thank you for this work, this was a very large patch with a whole lot of review comments, but I think what we ended up with is really great!

clang/include/clang/Sema/DeclSpec.h
424–431

It was definitely a great way to wake up this morning. "Wait, what? WHAT?!?" followed by a bunch of furious standards reading. :-D

clang/lib/AST/TypePrinter.cpp
1156–1157

That's fine by me!

clang/test/SemaCXX/type-traits.cpp
3505

This diagnostic is a bit unfortunate because it was given a non-bool integer, so I don't know that users will know how to fix the issue from the text. I leave it to your discretion where to split that diagnostic up with a %select so that _BitInt is special or not; I don't think users will hit this diagnostic all that often, but I do have a little bit of a worry about template metaprogramming accidentally getting into this state.

cjdb updated this revision to Diff 452285.Aug 12 2022, 1:06 PM

adds a clearer diagnostic for unsigned _BitInt(1)

cjdb updated this revision to Diff 452535.Aug 14 2022, 10:11 AM

moves TransformTypeTraits.def from clang/AST to clang/Basic

There are dowstream issues with having this definition file in the
former, because of the Bazel rules describing the relationship
between Basic and AST. Since Basic is the base and moving it there
is an NFC, I've taken the initiative to move it ahead of downstrem
usage. (Updating changelist for transperancy, but still merging
with main today.)

This revision was landed with ongoing or failed builds.Aug 14 2022, 10:34 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Aug 14 2022, 11:14 AM

This breaks building on windows: http://45.33.8.238/win/64423/step_4.txt

Please take a look and revert for now if it takes a while to fix.

This also broke building with GCC 9 on Ubuntu 20.04:

[5/225] ASTNodeAPI.json
FAILED: tools/clang/lib/Tooling/ASTNodeAPI.json
cd /home/martin/code/llvm-project/llvm/build/tools/clang/lib/Tooling && /home/martin/code/llvm-project/llvm/build/bin/clang-ast-dump --skip-processing=0 -I /home/martin/code/llvm-project/llvm/build/lib/clang/16.0.0/include -I /home/martin/code/llvm-project/llvm/tools/clang/include -I /home/martin/code/llvm-project/llvm/build/tools/clang/include -I /home/martin/code/llvm-project/llvm/build/include -I /home/martin/code/llvm-project/llvm/include -I /usr/include/c++/9 -I /usr/include/x86_64-linux-gnu/c++/9 -I /usr/include/c++/9/backward -I /usr/lib/gcc/x86_64-linux-gnu/9/include -I /usr/local/include -I /usr/include/x86_64-linux-gnu -I /usr/include --json-output-path /home/martin/code/llvm-project/llvm/build/tools/clang/lib/Tooling/ASTNodeAPI.json
clang-ast-dump: ../tools/clang/lib/Sema/DeclSpec.cpp:833: bool clang::DeclSpec::SetTypeSpecType(clang::DeclSpec::TST, clang::SourceLocation, const char*&, unsigned int&, const clang::PrintingPolicy&): Assertion `!isDeclRep(T) && !isTypeRep(T) && !isExprRep(T) && "rep required for these type-spec kinds!"' failed.
Aborted (core dumped)

Reverted this (and follow-ups) in aacf1a9742f714dd432117d82d19a007289c3dee for now.

cjdb reopened this revision.Aug 21 2022, 6:04 PM
This revision is now accepted and ready to land.Aug 21 2022, 6:04 PM
cjdb updated this revision to Diff 454350.Aug 21 2022, 6:06 PM

changes so this patch doesn't break things:

  1. changes from begin/end to data/size
  2. changes from ArrayRef to array since an ArrayRef to a prvalue is undefined (and causes segfaults when building with GCC)
This revision was landed with ongoing or failed builds.Aug 21 2022, 8:04 PM
This revision was automatically updated to reflect the committed changes.

I believe this patch might have broken LLDB bots https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/46269/

I've confirmed that d2d77e050b32 is good while e9ef45635b77 (this commit) breaks some tests.

To repro, a build of llvm+lldb+libcxx+libcxxapi and

# inside build dir
/bin/llvm-lit -v tools/lldb/test/API/lang/objc/modules-objc-property/TestModulesObjCProperty.py

Alternatively, running the check-lldb target should do it. But there is one other unrelated failure that might show up in that target

Forgot to add the failure message:

/usr/include/c++/v1/experimental/propagate_const:138:11: error: no template named 'remove_reference_t'; did you mean 'remove_reference'?
  typedef remove_reference_t<decltype(*_VSTD::declval<_Tp&>())> element_type;
cjdb added a comment.Aug 22 2022, 9:38 AM

Thanks for flagging this. Based on the fact it only seems to be remove_reference_t, I'm wondering if this is an issue with D131732 (which was merged immediately after D116203).

Thanks for flagging this. Based on the fact it only seems to be remove_reference_t, I'm wondering if this is an issue with D131732 (which was merged immediately after D116203).

I'm not sure, to expand on my previous message, I tested these two commits which appear one right after the other in the log:

  • e9ef45635b77 (14 hou..) cjdb@g.. [clang] adds unary type transformations as compiler built-ins
  • d2d77e050b32 (15 hou..) Ting.W.. [PowerPC][Coroutines] Add tail-call check with call information for coroutines

The tests pass on d2d77e050b32 and fail on e9ef45635b77

@cjdb Would you mind reverting the patch until we figured out a solution to unblock the CI?

cjdb added a comment.EditedAug 22 2022, 11:29 AM

@cjdb Would you mind reverting the patch until we figured out a solution to unblock the CI?

Sure, go ahead. I'm not even able to get to a point where I can repro the issue, as I get the following:

Command Output (stdout):
--
lldb version 16.0.0git (git@github.com:llvm/llvm-project.git revision 11ce014a121ed0bbdbb2af6ea20c75f85e89fbe9)
  clang revision 11ce014a121ed0bbdbb2af6ea20c75f85e89fbe9
  llvm revision 11ce014a121ed0bbdbb2af6ea20c75f85e89fbe9
Unable to load lldb extension module.  Possible reasons for this include:
  1) LLDB was built with LLDB_ENABLE_PYTHON=0
  2) PYTHONPATH and PYTHONHOME are not set correctly.  PYTHONHOME should refer to
     the version of Python that LLDB built and linked against, and PYTHONPATH
     should contain the Lib directory for the same python distro, as well as the
     location of LLDB's site-packages folder.
  3) A different version of Python than that which was built against is exported in
     the system's PATH environment variable, causing conflicts.
  4) The executable '/tmp/lldb-breaks/bin/lldb' could not be found.  Please check 
     that it exists and is executable.

--
Command Output (stderr):
--
Traceback (most recent call last):
  File "/home/cjdb/projects/llvm-bisect/lldb/test/API/dotest.py", line 7, in <module>
    lldbsuite.test.run_suite()
  File "/home/cjdb/projects/llvm-bisect/lldb/packages/Python/lldbsuite/test/dotest.py", line 892, in run_suite
    import lldb
ModuleNotFoundError: No module named 'lldb'

1 and 4 seem to be fine.

cjdb added a comment.Aug 22 2022, 2:43 PM

I think there's a forward fix available, but since I can't repro locally, I'm going to need to push it to main and observe how the tools fare.

I think there's a forward fix available, but since I can't repro locally, I'm going to need to push it to main and observe how the tools fare.

I need to be AFK for a bit now, but I can test locally on my end if you upload the patch here

This broke us in emscripten as well (building with trunk clang against a recent-but-not-trunk version of libcxx). I can test the fix if you want.

cjdb added a comment.Aug 22 2022, 4:17 PM

e137fb6fb85 should fix this issue. Thanks for your patience!