Page MenuHomePhabricator

[PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute
ClosedPublic

Authored by c-rhodes on Jul 10 2020, 6:05 AM.

Details

Summary

This patch implements 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 fixed-length (VLST) versions
of existing sizeless types (VLAT).

Implemented in this patch is the the behaviour described in section 3.7.3.2
and minimal parts of sections 3.7.3.3 and 3.7.3.4, this includes:

  • Defining VLST globals, structs, unions, and local variables
  • Implicit casting between VLAT <=> VLST.
  • Diagnosis of ill-formed conditional expressions of the form:

    C ? E1 : E2

    where E1 is a VLAT type and E2 is a VLST, or vice-versa. This avoids any ambiguity about the nature of the result type (i.e is it sized or sizeless).
  • For vectors:
    • sizeof(VLST) == N/8
    • alignof(VLST) == 16
  • For predicates:
    • sizeof(VLST) == N/64
    • alignof(VLST) == 2

VLSTs have the same representation as VLATs in the AST but are wrapped
with a TypeAttribute. Scalable types are currently emitted in the IR for
uses such as globals and structs which don't support these types, this
is addressed in the next patch with codegen, where VLSTs are lowered to
sized arrays for globals, structs / unions and arrays.

Not implemented in this patch is the behaviour guarded by the feature
macros:

  • __ARM_FEATURE_SVE_VECTOR_OPERATORS
  • __ARM_FEATURE_SVE_PREDICATE_OPERATORS

As such, the GNU attribute((vector_size)) extension is not available
and operators such as binary '+' are not supported for VLSTs. Support
for this is intended to be addressed by later patches.

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

Diff Detail

Event Timeline

c-rhodes created this revision.Jul 10 2020, 6:05 AM
sdesmalen added inline comments.Jul 13 2020, 6:34 AM
clang/include/clang/Basic/Attr.td
1543

nit: Can you add a comment saying why these are undocumented (and have no spellings)

clang/lib/AST/ASTContext.cpp
1872

Should this function just return unsigned and error when it doesn't have any of the ArmSveVectorBits attributes?
i.e. if isVLST() returns true, then it is an error if it doesn't have any of the attributes handled below.

1897

nit: I find this name a bit misleading, because I would expect the (ARM_SVE_VECTOR_) bits to be the same regardless of the type. Maybe rename this to getBitwidthForAttributedSveType ?

clang/lib/Sema/SemaType.cpp
7757–7762

Unrelated changes?

7812

If we only support power-of-two for now, we should only have an llvm_unreachable if we prevent parsing any of the other widths (and give an appropriate diagnostic saying those widths are not yet supported).

clang/test/Sema/attr-arm-sve-vector-bits.c
64

nit: For the tests that you've added below, can you add more elaborate comments explaining what you're trying to test?
e.g. here I assume that sizeless globals are otherwise not allowed, but they are when attributed with arm_sve_vector_bits. It would be good to have that explained a bit.

99

Is this diagnostic produced because of any code-changes in this patch?

155

nit: Is it necessary to test this for all the types?

225

nit: s/Test call/Test the scalable and fixed-width annotated types can be used interchangeably/

aaron.ballman added inline comments.Jul 14 2020, 12:48 PM
clang/include/clang/Basic/Attr.td
1543

Also, I think these are all missing let SemaHandler = 0; and let ASTNode = 0;

Is there a reason why we need N different type attributes instead of having a single type attribute which encodes the N as an argument? I think this may simplify the patch somewhat as you no longer need to switch over N as much.

sdesmalen added inline comments.Jul 14 2020, 1:21 PM
clang/include/clang/Basic/Attr.td
1543

Is there a reason why we need N different type attributes instead of having a single type attribute which encodes the N as an argument?

AIUI this was a workaround for getting the value of N from an AttributedType, because this only has getAttrKind to return the attribute kind, but no way to get the corresponding argument/value. This seemed like a simple way to do that without having to create a new subclass for Type and having to support that in various places. Is the latter the approach you were thinking of? (or is there perhaps a simpler way?)

c-rhodes added inline comments.Jul 14 2020, 4:38 PM
clang/include/clang/Basic/Attr.td
1543

Also, I think these are all missing let SemaHandler = 0; and let ASTNode = 0;

Good to know. In SemaType I'm doing CurType = State.getAttributedType(A, CurType, CurType); which gives an AttributedType in the AST, should I still set let ASTNode = 0; in this case?

Is there a reason why we need N different type attributes instead of having a single type attribute which encodes the N as an argument?

As Sander mentioned, it seemed like the easiest solution, interested to know if there's a better approach however

aaron.ballman added inline comments.Jul 15 2020, 4:46 AM
clang/include/clang/Basic/Attr.td
1543

I was thinking specifically of creating a new Type subclass and supporting it rather than adding 5 new attributes that only vary by a bit-width (which means there's likely to be a 6th someday). It's not immediately clear to me whether that's a really big ask for little gain or not, though.

1543

Ah, you're right, we may still need ASTNode to be kept around for that, good call.

c-rhodes updated this revision to Diff 278430.Thu, Jul 16, 4:42 AM

Changes:

  • Documented internal type attributes.
  • Set ASTNode = 0 on user-facing ArmSveVectorBitsAttr as the internal type attrs are used in the AST. Also removed the case for this from TypePrinter.
  • getSveVectorWidth now returns an unsigned. Added an unreachable if T has no attrs.
  • s/getArmSveVectorBits/getBitwidthForAttributedSveType. Also now returns an unsigned and asserts if !T->isVLST().
  • Add a few comments in test and reduced them a little so we dont tell all types for structs / unions etc.
c-rhodes marked 6 inline comments as done.Thu, Jul 16, 4:52 AM
c-rhodes added inline comments.
clang/include/clang/Basic/Attr.td
1543

Also, I think these are all missing let SemaHandler = 0; and let ASTNode = 0;

I've added let SemaHandler = 0; for the internal types and let ASTNode = 0; for the user-facing attr.

clang/lib/AST/ASTContext.cpp
1897

nit: I find this name a bit misleading, because I would expect the (ARM_SVE_VECTOR_) bits to be the same regardless of the type. Maybe rename this to getBitwidthForAttributedSveType ?

You're right, poor choice of name, I've updated it

clang/lib/Sema/SemaType.cpp
7757–7762

Unrelated changes?

TypeProcessingState &State is required to create an attributed type and Sema is attached to this so I removed it from the interface. This is inline with other attributed type handlers. I pulled S.Context out as it's used frequently.

7812

If we only support power-of-two for now, we should only have an llvm_unreachable if we prevent parsing any of the other widths (and give an appropriate diagnostic saying those widths are not yet supported).

Now that we check VecSize != S.getLangOpts().ArmSveVectorBits above I think this is ok as this shouldn't be reachable with -msve-vector-bits=<bits> validating the vector size.

clang/test/Sema/attr-arm-sve-vector-bits.c
99

Is this diagnostic produced because of any code-changes in this patch?

It isn't actually, I guess this could have gone in the first patch. Happy to move it if you think it's necessary.

155

nit: Is it necessary to test this for all the types?

I've reduced the number of types tested here.

c-rhodes added inline comments.Thu, Jul 16, 5:56 AM
clang/include/clang/Basic/Attr.td
1543

I was thinking specifically of creating a new Type subclass and supporting it rather than adding 5 new attributes that only vary by a bit-width (which means there's likely to be a 6th someday).

It would be nice if the Attr was accessible from the AttributedType, similar to how it is for Decls, so something like:

if (const auto *AT = T->getAs<AttributedType>())
  if (ArmSveVectorBitsAttr *Attr = AT->getAttr<ArmSveVectorBits>())
    unsigned Width = Attr->getNumBits();

Although I'm not sure if that makes sense or how easy it is. I do agree adding 5 new attributes isn't ideal but for an initial implementation it's nice and simple. Would you be ok with us addressing this in a later patch?

aaron.ballman added inline comments.Thu, Jul 16, 8:19 AM
clang/include/clang/AST/Type.h
1928

... this is a vector-length-sized type...

clang/include/clang/Basic/Attr.td
1543

It would be nice if the Attr was accessible from the AttributedType, similar to how it is for Decls, so something like:

You can do that through an AttributedTypeLoc object, which I think should be available from the situations you need to check this through a TypeSourceInfo * for the type. Then you can use AttributedTypeLoc::getAttr() to get the semantic attribute.

Would you be ok with us addressing this in a later patch?

No and yes. It's a novel design to have a user-facing attribute that is never hooked up in the AST but is instead used to decide which spellingless attribute to use instead, so I'd strongly prefer to find a solution that doesn't use this approach. I also think we'll wind up with cleaner code from this. That said, if it turns out to be awkward to do because there's too much code required to support it, then I can likely hold my nose.

c-rhodes added inline comments.Thu, Jul 16, 2:02 PM
clang/include/clang/Basic/Attr.td
1543

You can do that through an AttributedTypeLoc object, which I think should be available from the situations you need to check this through a TypeSourceInfo * for the type. Then you can use AttributedTypeLoc::getAttr() to get the semantic attribute.

I've tried your suggestion with the following (in getSveVectorWidth):

TypeSourceInfo *TInfo = getTrivialTypeSourceInfo(QualType(T, 0));
TypeLoc TL = TInfo->getTypeLoc();
if (AttributedTypeLoc Attr = TL.getAs<AttributedTypeLoc>())
  if (const auto *AT = Attr.getAttrAs<ArmSveVectorBitsAttr>())
    llvm::outs() << AT->getNumBits() << "\n";

but can't seem to get it to work as there's no Attr attached to the AttributedTypeLoc. In this function in ASTContext all I have is a Type, I'm not sure if I'm missing something obvious.

I also tried it in HandleArmSveVectorBitsTypeAttr after creating the AttributedType to see how it would work and the following does allow me to query to vector size with getNumBits:

auto *A =
    ::new (Ctx) ArmSveVectorBitsAttr(Ctx, Attr, static_cast<unsigned>(VecSize));
A->setUsedAsTypeAttr();

CurType = State.getAttributedType(A, CurType, CurType);
TypeSourceInfo *TInfo = Ctx.CreateTypeSourceInfo(CurType);
TypeLoc TL = TInfo->getTypeLoc();
if (AttributedTypeLoc ATL = TL.getAs<AttributedTypeLoc>()) {
  ATL.setAttr(A);
  if (const auto *AT = ATL.getAttrAs<ArmSveVectorBitsAttr>())
    llvm::outs() << AT->getNumBits() << "\n";
}

Although I had to explicitly call ATL.setAttr(A);. It seems like the TypeSourceInfo is missing something, for reference the QualType I'm passing looks:

AttributedType 0x1946910 'svint8_t __attribute__((arm_sve_vector_bits))' sugar
|-TypedefType 0x19468a0 'svint8_t' sugar
| |-Typedef 0x19235c8 'svint8_t'
| `-TypedefType 0x1923590 '__SVInt8_t' sugar
|   |-Typedef 0x1921598 '__SVInt8_t'
|   `-BuiltinType 0x1881460 '__SVInt8_t'
`-TypedefType 0x19468a0 'svint8_t' sugar
  |-Typedef 0x19235c8 'svint8_t'
  `-TypedefType 0x1923590 '__SVInt8_t' sugar
    |-Typedef 0x1921598 '__SVInt8_t'
    `-BuiltinType 0x1881460 '__SVInt8_t'

I think I'll see if it's any easier to create a new Type.

aaron.ballman added inline comments.Fri, Jul 17, 7:02 AM
clang/include/clang/Basic/Attr.td
1543

but can't seem to get it to work as there's no Attr attached to the AttributedTypeLoc.

That's surprising to me and smells like a bug. IIRC, the attribute is set in the AttributedTypeLoc object within the TypeSpecLocFiller builder in SemaType.cpp. Perhaps you could see whether that's being called properly before the call to ASTContext::getTypeInfoImpl()?

I think I'll see if it's any easier to create a new Type.

That's a reasonable fallback solution, but I'd like to understand why the existing machinery isn't workable and see if we can use that instead.

c-rhodes added inline comments.Sat, Jul 18, 10:39 AM
clang/include/clang/Basic/Attr.td
1543

That's surprising to me and smells like a bug. IIRC, the attribute is set in the AttributedTypeLoc object within the TypeSpecLocFiller builder in SemaType.cpp. Perhaps you could see whether that's being called properly before the call to ASTContext::getTypeInfoImpl()?

Thanks for the pointers, I verified the Attr is being set for the AttributedTypeLoc in VisitAttributedTypeLoc (via fillAttributedTypeLoc) and I think this is attached to the TypeSourceInfo in GetTypeSourceInfoForDeclarator that is ultimately used to create a TypedefDecl in ActOnTypedefDeclarator. I can't see how it's possible to get an AttributedTypeLoc in ASTContext from the AttributedType alone. The TypeSpecLocFiller builder in SemaType.cpp is filling the TypeSourceInfo in for the decl and the only option I have in ASTContext is to create a new TypeSourceInfo.

That's a reasonable fallback solution, but I'd like to understand why the existing machinery isn't workable and see if we can use that instead.

Another alternative solution could be to get the vector size from the language options and have a single ArmSveVectorBits attribute. The only place we need to really query this is in ASTContext where we have access to the lang options. It would be nice to have this information in the TypePrinter as well but I think that's easily doable through the PrintingPolicy, although I'm not sure if that's really necessary. Would that be an acceptable approach?

c-rhodes updated this revision to Diff 279178.Mon, Jul 20, 4:22 AM

Changes:

  • Remove internal type attributes (defined for each vector-size).
  • Get the vector size from the arm_sve_vector_bits attribute via the AttributedTypeLoc associated with the typedef decl.
  • Change NumBits argument for ArmSveVectorBits type attribute from int to unsigned.
  • Only allow ArmSveVectorBits type attribute to be applied to typedefs (and added test).
  • Set let PragmaAttributeSupport = 0; after specifying Subjects to fixed clang/test/Misc/pragma-attribute-supported-attributes-list.test.
  • vector-length sized -> vector-length-sized.
c-rhodes marked an inline comment as done.Mon, Jul 20, 4:34 AM
c-rhodes added inline comments.
clang/include/clang/Basic/Attr.td
1543

I can't see how it's possible to get an AttributedTypeLoc in ASTContext from the AttributedType alone. The TypeSpecLocFiller builder in SemaType.cpp is filling the TypeSourceInfo in for the decl and the only option I have in ASTContext is to create a new TypeSourceInfo

@sdesmalen suggested calling getBitwidthForAttributedSveType for Type::Typedef (rather than AttributedType) in getTypeInfoImpl which works! From there the TypeSourceInfo can be pulled from the TypedefDecl I mentioned.

I like the way the code is shaping up with the slightly altered design, nice!

clang/lib/AST/ASTContext.cpp
1872

Should declare the method as static.

1887

Should this be dividing by the number of bits in a char for the target as opposed to hard-coding to 8?

c-rhodes added inline comments.Mon, Jul 20, 9:36 AM
clang/lib/AST/ASTContext.cpp
1887

Should this be dividing by the number of bits in a char for the target as opposed to hard-coding to 8?

Predicate registers in SVE hold one bit per byte of a vector register so each predicate is 1/8th the size of a vector which are defined in bits, it has to be 8 and I know getCharWidth returns 8 for the target this is implemented for but I dont know what it would mean for any other target or if we care about that?

c-rhodes updated this revision to Diff 279455.Tue, Jul 21, 2:28 AM
  • Make helpers static in ASTContext.
  • Use getCharWidth.
  • s/vector-length-sized/vector-length-specific.
c-rhodes marked 2 inline comments as done.Tue, Jul 21, 2:29 AM
aaron.ballman accepted this revision.Tue, Jul 21, 12:05 PM

The attribute bits LGTM, thanks!

clang/lib/AST/ASTContext.cpp
1887

Ah, so this is a case where we expect that value to always be 8 where this feature is supported anyway -- good to know. I still think the current code is a cleaner approach to hard-coding 8 bits, so thank you for making the change even though it doesn't enable much.

This revision is now accepted and ready to land.Tue, Jul 21, 12:05 PM

The attribute bits LGTM, thanks!

Thanks for reviewing!

This revision was automatically updated to reflect the committed changes.