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

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes marked an inline comment as done.
c-rhodes added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1541
 
+def ArmSveVectorBits128 : TypeAttr {
+  let Spellings = [];
----------------
c-rhodes wrote:
> aaron.ballman wrote:
> > c-rhodes wrote:
> > > aaron.ballman wrote:
> > > > c-rhodes wrote:
> > > > > c-rhodes wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > c-rhodes wrote:
> > > > > > > > > sdesmalen wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > sdesmalen wrote:
> > > > > > > > > > > > nit: Can you add a comment saying why these are undocumented (and have no spellings)
> > > > > > > > > > > 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.
> > > > > > > > > > > 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?)
> > > > > > > > > > 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
> > > > > > > > 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.
> > > > > > > Ah, you're right, we may still need `ASTNode` to be kept around for that, good call.
> > > > > > > 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.
> > > > > > 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 `Decl`s, 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?
> > > > > 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.
> > > > 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`.
> > > 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.
> > 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?
> 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83551/new/

https://reviews.llvm.org/D83551