aaron.ballman added inline comments.
================
Comment at: clang/include/clang/AST/Type.h:1928
+ /// Determines if this is vector-length sized typed (VLST), i.e. a
+ /// sizeless type with the 'arm_sve_vector_bits(N)' attribute applied.
----------------
... this is a vector-length-sized type...
================
Comment at: clang/include/clang/Basic/Attr.td:1541
+def ArmSveVectorBits128 : TypeAttr {
+ let Spellings = [];
----------------
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.
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
