This is an archive of the discontinued LLVM Phabricator instance.

Clang implementation of sizeless types
AbandonedPublic

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

Details

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.

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
29 ↗(On Diff #203383)

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

test/SemaCXX/sizeless-1.cpp
37 ↗(On Diff #203383)

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
60 ↗(On Diff #203546)

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
60 ↗(On Diff #203546)

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

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

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

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.

tlively added a subscriber: tlively.Oct 7 2019, 1:21 PM
khchen added a subscriber: khchen.Feb 6 2020, 8:00 AM

Update to latest master. Make isIncompleteType return true for sizeless
types here rather than in D62961.

Can you make a list of the complete stack of patches here?

Can you make a list of the complete stack of patches here?

Sorry for the slow reply, I was out last week. I've updated the stack for this revision: the only parent patch is D62961. As you'll have noticed from the spam (sorry), I've also uploaded the start of an alternative series of patches that achieve the same thing. The original series (this one) started by making the SVE types incomplete and then enabled things that are actually valid for SVE types. The new series instead starts from the status quo, where we accept all valid code and lots of invalid code. It then gradually forbids all the known invalid uses. The new series also avoids the "definite/indefinite" classification and just sticks to sized/sizeless and complete/incomplete.

The start of the new stack is D75570 and the current end is D75573, but the full series currently has 23 patches. All patches but the last one are in a similar style to D75572 and D75573.

Okay, thanks.

pmatos added a subscriber: pmatos.Jul 10 2020, 2:35 AM
rsandifo-arm abandoned this revision.Nov 27 2020, 2:36 AM

Was superceded by later revisions, but I forgot to close this one.

clang/lib/Sema/SemaType.cpp