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

[Cullen Rhodes via Phabricator via cfe-commits]

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