aaron.ballman 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:
> > > 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.
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
