c-rhodes added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:1541
+def ArmSveVectorBits128 : TypeAttr {
+ let Spellings = [];
----------------
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`.
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
