[PATCH] D83551: [PATCH 2/4][Sema][AArch64] Add semantics for arm_sve_vector_bits attribute

[Aaron Ballman via Phabricator via cfe-commits]

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