Page MenuHomePhabricator

Clang implementation of sizeless types
Needs ReviewPublic

Authored by rsandifo-arm on Jun 6 2019, 8:48 AM.

Details

Reviewers
rengolin
Summary

This patch adds the concept of "sizeless" types to C and C++.
The documentation part of the patch describes the extension in
detail and explains the rationale. The patch is a prerequisite
for adding AArch64 SVE intrinsic functions.

The main change is to make several checks use the new distinction
between "definite" and "indefinite" types instead of the usual
distinction between "complete" and "incomplete" types. There are
also a couple of places that check specifically for sizeless types.

The patch doesn't reword the diagnostics to talk about indefinite
types rather than incomplete types since (a) "indefinite" won't
mean much to most users and (b) the patch shouldn't affect the user
experience for non-AArch64 targets.

The FIXMEs about inline asms are resolved by later SVE patches.

The change to DiagnoseForRangeVariableCopies is for consistency.
As the tests show, DiagnoseForRangeReferenceVariableCopies can apply
to sizeless types, but at the moment DiagnoseForRangeConstVariableCopies
(which is the only part affected by the completeness check) doesn't
warn for PODs, and so doesn't warn for sizeless types either.

Diff Detail

Event Timeline

rsandifo-arm created this revision.Jun 6 2019, 8:48 AM
jaykang10 added inline comments.Jun 7 2019, 2:28 AM
test/Sema/sizeless-1.c
30

Please check "attribute((ext_vector_type()))". I guess it is also invalid for this type.

test/SemaCXX/sizeless-1.cpp
38

Please check "attribute((ext_vector_type()))". I guess it is also invalid for this type.

Add tests for ext_vector_type

miyuki added a subscriber: miyuki.Jun 7 2019, 8:27 AM
jfb added a comment.Jun 7 2019, 12:28 PM

How do these types mangle?

test/Sema/sizeless-1.c
61

What do you expect these to return?

Improve atomic built-in tests.

rsandifo-arm marked 4 inline comments as done.Jun 10 2019, 1:57 AM

Thanks for the review.

In D62962#1534528, @jfb wrote:

How do these types mangle?

At the moment, the mangling is only defined for the Itanium ABI, which uses str(len(name)) + name for the built-in type name (e.g. __SVInt8_t. This is implemented in https://reviews.llvm.org/D62960.

test/Sema/sizeless-1.c
61

Fixed to make this clearer. The types provide no compile-time alignment guarantees, so constant evaluation needs to make the most pessimistic assumption.

jfb added inline comments.Jun 10 2019, 10:13 AM
test/Sema/sizeless-1.c
66

I expect sizeless types are never lock free.

rsandifo-arm marked 2 inline comments as done.Jun 10 2019, 10:57 AM
rsandifo-arm added inline comments.
test/Sema/sizeless-1.c
66

Yeah, that's right. But AIUI __atomic_always_lock_free(N, P) asks whether an N-byte access at P is lock-free (where P can be null to query standard alignment). So the question isn't whether sizeless types are lock-free, but whether an N-byte access is lock-free given the alignment guarantees of P. For this line the answer would be yes if &local_int8 was aligned to a 2-byte boundary.

The query isn't really that interesting for sizeless types. The reason for having it is that IntExprEvaluator::VisitBuiltinCallExpr says that 1-byte accesses are lock-free if the target has lock-free accesses, whatever P happens to be. But for larger N it punts when P is a pointer to incomplete type, because in that case it knows nothing about the alignment of P. The test is enforcing this behaviour for sizeless types too.

jfb added inline comments.Jun 10 2019, 11:06 AM
test/Sema/sizeless-1.c
66

Ah yes, that makes sense. Thanks!

Ka-Ka added a subscriber: Ka-Ka.Jun 11 2019, 7:06 AM

Since this is an extension, it wouldb be great to have a (on-by-default? at least in -Wall) diagnostic for every instance of usage of this extension.
(I guess this is actually true for clang vector extension, too..)

Since this is an extension, it wouldb be great to have a (on-by-default? at least in -Wall) diagnostic for every instance of usage of this extension.

We generally only add warnings for extensions which are not allowed by the standard. It's impossible to define a sizeless type without using a reserved keyword, like __SVInt8_t, so we don't need a warning.