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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
