Page MenuHomePhabricator

[Sema][AArch64] Support arm_sve_vector_bits attribute
ClosedPublic

Authored by c-rhodes on Aug 11 2020, 7:59 AM.

Details

Summary

This patch implements the semantics for the 'arm_sve_vector_bits' type
attribute, defined by the Arm C Language Extensions (ACLE) for SVE [1].
The purpose of this attribute is to define vector-length-specific (VLS)
versions of existing vector-length-agnostic (VLA) types.

The semantics were already implemented by D83551, although the
implementation approach has since changed to represent VLSTs as
VectorType in the AST and fixed-length vectors in the IR everywhere
except in function args/returns. This is described in the prototype
patch D85128 demonstrating the new approach.

The semantic changes added in D83551 are changed since the
AttributedType is replaced by VectorType in the AST. Minimal changes
were necessary in the previous patch as the canonical type for both VLA
and VLS was the same (i.e. sizeless), except in constructs such as
globals and structs where sizeless types are unsupported. This patch
reverts the changes that permitted VLS types that were represented as
sizeless types in such circumstances, and adds support for implicit
casting between VLA <-> VLS types as described in section 3.7.3.2 of the
ACLE.

Since the SVE builtin types for bool and uint8 are both represented as
BuiltinType::UChar in VLSTs, two new vector kinds are implemented to
distinguish predicate and data vectors.

[1] https://developer.arm.com/documentation/100987/latest

Diff Detail

Event Timeline

c-rhodes created this revision.Aug 11 2020, 7:59 AM
c-rhodes requested review of this revision.Aug 11 2020, 7:59 AM
c-rhodes updated this revision to Diff 285124.Aug 12 2020, 10:07 AM

Added missing implicit conversions for C++. I considered handling this with the
existing implicit vector conversion although one side of the conversion will be
an SVE builtin, so instead I've added a new conversion specifically for SVE. I
suspect the existing one could support this but I wasn't sure if that was a good
idea (?). In C++ implicit conversions between VLA/VLS have a rank just below
derived-to-base conversion.

rsandifo-arm added inline comments.Aug 12 2020, 10:50 AM
clang/include/clang/AST/Type.h
1897

It feels to me like the information is more general than that, since the type of the element is meaningful independently of the new attribute. How about just getSveEltType instead?

clang/lib/AST/ASTContext.cpp
1941

The alignment of the SVE data vectors should be 128 too (see the alignof stuff in the ACLE doc). FWIW, there were two reasons for defining it like that:

  • the fixed-length types map to the same ABI machine type as the variable-length types
  • the length isn't required to be a power of 2: an implementation that supports fixed-length 384-bit vectors could define __ARM_FEATURE_SVE_BITS to 384.
clang/lib/AST/TextNodeDumper.cpp
1413

Is it worth distinguishing these, similarly to altivec?

clang/lib/AST/Type.cpp
2338

This is just a note, not sure what can be done about it, but:

The element types are defined in terms of the stdint.h types. One snag is that ILP32 newlib defines int32_t to be UnsignedLongTy instead of UnsignedIntTy, and defines uint64_t to UnsignedLongLongTy.

In GCC we got away with this because GCC already has hard-coded knowledge about the target C library's choices. I don't think that information is available here though.

Like I say, there's nothing that necessarily needs to be “fixed”/dealt-with now, just thought it was worth mentioning.

clang/lib/Sema/SemaExpr.cpp
8907

I guess this is personal preference, but it seems more natural to use [&] and not pass the context. Maybe different names from LHSType and RHSType would be better for the nested function, since it's called with both orders.

8915

Is the isVector just being defensive? I'd have expected it to be redundant, since we shouldn't create SveFixedLengthPredicateVectors with invalid element types.

8918

It looks like this could still trigger for SveBools. Maybe it would be better to have:

if (BT->getKind() == BuiltinType::SveBool) {
  …
} else {
  …
}

instead. Perhaps it'd also be worth having an assert to show that we've already checked that any builtin type is an SVE type.

9914

Seems like it might be useful to have a helper function for testing this pair of vector kinds.

c-rhodes updated this revision to Diff 285285.Aug 13 2020, 1:53 AM
c-rhodes marked an inline comment as not done.

Address @rsandifo-arm comments.

c-rhodes marked 4 inline comments as done.Aug 13 2020, 2:19 AM
c-rhodes added inline comments.
clang/lib/AST/ASTContext.cpp
1941

The alignment of the SVE data vectors should be 128 too (see the alignof stuff in the ACLE doc).

Ah, good point. This would've bitten us when when we come to supporting non-power-of-2 vector lengths.

clang/lib/AST/Type.cpp
2338

Thanks for the heads up! I'm not familiar with ILP32 so that's good to know. So if -mabi=ilp32 is passed then I guess we should handle that here. I'll make a note of this and address it in a later patch.

clang/lib/Sema/SemaExpr.cpp
8907

I guess this is personal preference, but it seems more natural to use [&] and not pass the context.

Agreed, that's nicer.

Maybe different names from LHSType and RHSType would be better for the nested function, since it's called with both orders.

I've renamed them to first/second.

8915

Is the isVector just being defensive? I'd have expected it to be redundant, since we shouldn't create SveFixedLengthPredicateVectors with invalid element types.

Yeah although as you say it's not really necessary, fixed.

8918

I've simplified this somewhat although I'm not sure if it's what you meant. If it's a predicate vector kind then it's compatible if the builtin is an SVE bool, otherwise it compares the types. Worth noting I've moved this to ASTContext also since it's used for the C++ conversions.

LGTM from a spec point of view, but I don't think I should be the one to approve.

aaron.ballman accepted this revision.Aug 26 2020, 9:11 AM

LGTM aside from some minor nits.

clang/include/clang/AST/Type.h
3228

Add a bit of vertical whitespace for consistency with surrounding enumerators?

clang/include/clang/Basic/DiagnosticSemaKinds.td
2928

vector -> vectors
(alternatively, a fixed-length and a sizeless vector)

clang/lib/AST/ASTContext.cpp
1945

else if?

8392

This only needs to capture this and not reference capture everything, right? I'd prefer that (esp given that the lambda parameters shadow the names of the function parameters).

clang/lib/Sema/SemaExpr.cpp
9913

No need for the parameters to be non-const references (you can use value types here, they're cheap).

clang/lib/Sema/SemaOverload.cpp
1656

This can be hoisted into the preceding if statement.

clang/lib/Sema/SemaType.cpp
7817

Full stop at the end of the comment.

This revision is now accepted and ready to land.Aug 26 2020, 9:11 AM
c-rhodes updated this revision to Diff 288250.Aug 27 2020, 2:45 AM

Address comments

c-rhodes marked 7 inline comments as done.Aug 27 2020, 2:47 AM

@rsandifo-arm @aaron.ballman thanks for reviewing!

This revision was automatically updated to reflect the committed changes.