This is an archive of the discontinued LLVM Phabricator instance.

Add SVE opaque built-in types
ClosedPublic

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

Details

Summary

This patch adds the SVE built-in types defined by the Procedure Call
Standard for the Arm Architecture:

https://developer.arm.com/docs/100986/0000

It handles the types in all relevant places that deal with built-in types.
At the moment, some of these places bail out with an error, including:

(1) trying to generate LLVM IR for the types
(2) trying to generate debug info for the types
(3) trying to mangle the types using the Microsoft C++ ABI
(4) trying to @encode the types in Objective C

(1) and (2) are fixed by follow-on patches but (unlike this patch)
they deal mostly with target-specific LLVM details, so seemed like
a logically separate change. There is currently no spec for (3) and
(4), so reporting an error seems like the correct behaviour for now.

The intention is that the types will become sizeless types:

http://lists.llvm.org/pipermail/cfe-dev/2019-June/062523.html

The main purpose of the sizeless type extension is to diagnose
impossible or dangerous uses of the types, such as any that would
require sizeof to have a meaningful defined value.

Until then, the patch sets the alignments of the types to the values
specified in the link above. It also sets the sizes of the types to
zero, which is chosen to be consistently wrong and shouldn't affect
correctly-written code (i.e. code that would compile even with the
sizeless type extension).

The patch adds the common subset of functionality needed to test the
sizeless type extension on the one hand and to provide SVE intrinsic
functions on the other. After this patch, the two pieces of work are
essentially independent.

The patch is based on one by Graham Hunter:

https://reviews.llvm.org/D59245

Diff Detail

Repository
rL LLVM

Event Timeline

rsandifo-arm created this revision.Jun 6 2019, 8:45 AM

Tests?

lib/AST/ItaniumMangle.cpp
2680 ↗(On Diff #203379)

@rjmccall you probably should review this part.

erik.pilkington added inline comments.
lib/AST/ItaniumMangle.cpp
2680 ↗(On Diff #203379)

Sorry for the drive by comment, but: All of these mangling should really be using the "vendor extension" production IMO:

<type> ::= u <source-name>

As is, these manglings intrude on the users's namespace, (i.e. if they had a type named objc_selector or something), and confuse demanglers which incorrectly assume these are substitutable (vendor extension builtin types are substitutable too though, but that should be handled here).

Ka-Ka added a subscriber: Ka-Ka.Jun 11 2019, 11:39 PM
rsandifo-arm marked an inline comment as done.Jun 19 2019, 7:13 AM
rsandifo-arm added inline comments.
lib/AST/ItaniumMangle.cpp
2680 ↗(On Diff #203379)

It isn't obvious from the patch, but the SVE names that we're mangling are predefined names like __SVInt8_t. rather than user-facing names like svint8_t The predefined names and their mangling are defined by the platform ABI (https://developer.arm.com/docs/100986/0000), so it wouldn't be valid for another part of the implementation to use those names for something else.

I realise you were making a general point here though, sorry.

lib/AST/ItaniumMangle.cpp
2680 ↗(On Diff #203379)

The mangling in the document you linked does use the vendor extension production here though, i.e. the example is void f(int8x8_t), which mangles to _Z1fu10__Int8x8_t. It is true that this shouldn't ever collide with another mangling in practice, but my point is there isn't any need to smuggle it into the mangling by pretending it's a user defined type, when the itanium grammar and related tools have a special way for vendors to add builtin types.

rovka added a subscriber: rovka.Jun 27 2019, 4:59 AM

Just a few nits/suggestions.

include/clang/Basic/AArch64SVEACLETypes.def
10 ↗(On Diff #203379)

You can be more specific :)

29 ↗(On Diff #203379)

Maybe use SVE_TYPE instead of BUILTIN_TYPE, to avoid any confusion? You can't re-use a defition of BUILTIN_TYPE between this header and BuiltinTypes.def anyway, since they both undefine it at the end.

include/clang/Serialization/ASTBitCodes.h
1021 ↗(On Diff #203379)

super nit: /// \brief SVE types with auto numeration

lib/AST/ASTContext.cpp
6645 ↗(On Diff #203379)

Here and in most other places: no need for 2 defines, you can just #define BUILTIN_TYPE (or if you choose to rename it to SVE_TYPE) since it's the same code for both vectors and predicates.

lib/AST/ItaniumMangle.cpp
2680 ↗(On Diff #203379)

I agree with Erik here, the example in the PCS document seems to suggest using u. I think either the patch needs to be updated or the document needs to be more clear about what the mangling is supposed to look like.

lib/AST/MicrosoftMangle.cpp
2110 ↗(On Diff #203379)

Should we have llvm_unreachable or report a proper error? I like the unreachable if we're checking elsewhere that SVE isn't supported on Windows, but I notice we report errors for some of the other types.

lib/CodeGen/CGDebugInfo.cpp
709 ↗(On Diff #203379)

I don't really know this code, but I can't help but notice that nullptr is only ever used for the void type. Is it safe to also use it for the SVE types, or can we do something else instead?

  • Fix comments in AArch64SVEACLETypes.def
  • Rename BUILTIN_TYPE to SVE_TYPE and use it where possible
  • Report errors for TODOs instead of using llvm_unreachable
  • Add a test for the errors
  • Formatting fixes
rsandifo-arm marked 7 inline comments as done.Jun 27 2019, 11:20 AM

Thanks for the reviews!

include/clang/Basic/AArch64SVEACLETypes.def
10 ↗(On Diff #203379)

Yeah, there were a few cut-&-pastos here, sorry. Hopefully fixed in the updated version.

29 ↗(On Diff #203379)

Yeah, that's definitely better, especially given the different number of arguments. Done in the updated version.

lib/AST/ASTContext.cpp
6645 ↗(On Diff #203379)

Thanks, that shaved quite a lot of lines off the patch :-)

lib/AST/ItaniumMangle.cpp
2680 ↗(On Diff #203379)

Thanks for highlighting this problem, and sorry for not noticing myself when pointing you at the doc.

Unfortunately, the specification and implementation already difer for the Advanced SIMD types, with both clang and GCC omitting the 'u' despite the spec saying it should be present. So we're considering changing the spec to match what's now the de facto ABI.

For SVE we do still have the opportunity to use 'u'. I've left it as-is for now though, until we've reached a decision about whether to follow existing practice for Advanced SIMD or whether to do what the spec says.

lib/AST/MicrosoftMangle.cpp
2110 ↗(On Diff #203379)

Fixed to report the error, since this wouldn't be trapped earlier.

lib/CodeGen/CGDebugInfo.cpp
709 ↗(On Diff #203379)

Fixed to report an error here too and return a "safe" value until the TODO is fixed. Also added a test.

rovka added a comment.Jul 2 2019, 3:21 AM

This looks much better, thanks! Shouldn't there be more tests, e.g. for mangling and maybe the ASTImporter?

Changes since last version:

  • Define the SVE types only for AArch64 targets
  • Use the vendor extension 'u' mangling, as per the spec
  • Report an error for @encode on the types
  • Fixed the licence text on the new file
  • Added more tests

Although I'd originally posted the patch so that the sizeless type
support had specific types to test, it'd be really useful if the patch
could go in independently of the sizeless type stuff, since the
patch contains the common subset of functionality needed by the
sizeless type patches and by the implementation of the intrinsics
(which apart from this patch are essentially separate pieces of work).

rsandifo-arm retitled this revision from SVE opaque type for C intrinsics demo to Add SVE opaque built-in types.Jul 4 2019, 1:57 AM
rsandifo-arm edited the summary of this revision. (Show Details)

This looks much better, thanks! Shouldn't there be more tests, e.g. for mangling and maybe the ASTImporter?

Thanks. I've added tests for ASTImporter and mangling in the latest version, as well as tests for typeinfo, AST dumping, Objective C errors, PCH serialisation and (until we diagnose them as an error) sizeof/alignof queries.

The ASTImporter and the test for it looks good to me, thanks!

martong resigned from this revision.Jul 4 2019, 2:34 AM
rjmccall added inline comments.Jul 4 2019, 12:24 PM
lib/AST/ItaniumMangle.cpp
2680 ↗(On Diff #203379)

These do seem more "builtin" than the SIMD types, but I don't think it deeply matters either way, since these are already reserved names.

Ping, and thanks for the reviews so far. I think I've addressed all the comments to date and I've tried to make the patch ready to commit in its current state (rather than the RFC that it originally was).

It would be really useful if we could apply the patch soon, so that we can start implementing the intrinsic functions. The patch is now independent of the "sizeless type" stuff, if there are concerns about that.

Seems reasonable to me.

rovka added a comment.Jul 19 2019, 2:40 AM

FWIW, I think the tests look great. Would be nice if someone more experienced with clang could also have a look though.

rsandifo-arm marked 10 inline comments as done.Jul 30 2019, 12:19 PM

Thanks. I think the current version of the patch addresses all review comments so far and the last set of comments seemed positive. Does the patch look OK to land?

(I was waiting until Clang 9 branched, and then was away last week, hence the long delay...)

Ping.

@rjmccall: thanks for the positive feedback. Is there a particular reviewer you'd like to hear from before the patch lands?

rjmccall added inline comments.Aug 7 2019, 2:39 PM
lib/AST/ASTContext.cpp
1974 ↗(On Diff #207991)

Why do SVE predicates have 16-bit alignment? Should this be 128-bit (16-*byte*)?

I guess these alignments are reasonable to hard-code here since they're target-specific for now. That might be worth including in the comment.

lib/AST/ItaniumMangle.cpp
2680 ↗(On Diff #203379)

Did you actually make the decision to use the u mangling?

lib/AST/MicrosoftMangle.cpp
2073 ↗(On Diff #207991)

Comment isn't adding anything here.

lib/AST/NSAPI.cpp
487 ↗(On Diff #207991)

In this and other places, please follow nearby style about whether to place the case expansion on the following line, as is done above for the OpenCL extension types.

lib/CodeGen/CGDebugInfo.cpp
709 ↗(On Diff #203379)

This is fine as long as you're planning to at least fill in a hacky implementation when you implement the rest of IRGen support.

rsandifo-arm edited the summary of this revision. (Show Details)
  • Remove pointless "SVE Types" comments
  • Expand commentary about SVE type layout and future debug info handling
  • Make macro formatting more consistent with the surrounding code
rsandifo-arm marked 7 inline comments as done.Aug 8 2019, 3:12 AM
rsandifo-arm added inline comments.
lib/AST/ASTContext.cpp
1974 ↗(On Diff #207991)

Yeah, in retrospect this is sorely lacking a comment. The reason for using 16 is that there is one predicate bit for each vector byte, so the predicate size is a runtime multiple of 16 bits. I've added a comment to say that and added a reference to the ABI that defines the alignments.

lib/AST/ItaniumMangle.cpp
2680 ↗(On Diff #203379)

Yeah, we decided to stick with the 'u' as documented for SVE. (It's too late for Advanced SIMD unfortunately.)

lib/AST/MicrosoftMangle.cpp
2073 ↗(On Diff #207991)

I've removed these comments from everything except Type.h, where having them matches the surrounding style.

lib/CodeGen/CGDebugInfo.cpp
709 ↗(On Diff #203379)

Hopefully it won't be too hacky, at least not in its final form. :-) I've added a comment outlining how we handle debug info for these types, but explained why it isn't included yet.

rjmccall accepted this revision.Aug 8 2019, 9:14 AM

LGTM.

lib/AST/ASTContext.cpp
1974 ↗(On Diff #207991)

Thanks!

I'm surprised predicates don't require a higher alignment, even if it'd be excessive for very short vectors, but if that's the ABI rule, so be it.

This revision is now accepted and ready to land.Aug 8 2019, 9:14 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2019, 1:52 AM