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:
> > 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?
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
