[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 = [];
----------------
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