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

[Cullen Rhodes via Phabricator via cfe-commits]

c-rhodes marked 6 inline comments as done.
c-rhodes added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1541
 
+def ArmSveVectorBits128 : TypeAttr {
+  let Spellings = [];
----------------
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.


================
Comment at: clang/lib/AST/ASTContext.cpp:1897
+
+bool ASTContext::getArmSveVectorBits(const Type *T, unsigned &Width) const {
+  if (!T->isVLST())
----------------
sdesmalen wrote:
> nit: I find this name a bit misleading, because I would expect the (ARM_SVE_VECTOR_) bits to be the same regardless of the type. Maybe rename this to `getBitwidthForAttributedSveType` ?
> nit: I find this name a bit misleading, because I would expect the (ARM_SVE_VECTOR_) bits to be the same regardless of the type. Maybe rename this to getBitwidthForAttributedSveType ?

You're right, poor choice of name, I've updated it


================
Comment at: clang/lib/Sema/SemaType.cpp:7754
 /// the ACLE, such as svint32_t and svbool_t.
-static void HandleArmSveVectorBitsTypeAttr(QualType &CurType,
-                                           const ParsedAttr &Attr, Sema &S) {
+static void HandleArmSveVectorBitsTypeAttr(TypeProcessingState &State,
+                                           QualType &CurType,
----------------
sdesmalen wrote:
> Unrelated changes?
> Unrelated changes?

`TypeProcessingState &State` is required to create an attributed type and `Sema` is attached to this so I removed it from the interface. This is inline with other attributed type handlers. I pulled `S.Context` out as it's used frequently.


================
Comment at: clang/lib/Sema/SemaType.cpp:7839
+  default:
+    llvm_unreachable("unsupported vector size!");
+  case 128:
----------------
sdesmalen wrote:
> If we only support power-of-two for now, we should only have an `llvm_unreachable` if we prevent parsing any of the other widths (and give an appropriate diagnostic saying those widths are not yet supported).
> If we only support power-of-two for now, we should only have an llvm_unreachable if we prevent parsing any of the other widths (and give an appropriate diagnostic saying those widths are not yet supported).

Now that we check `VecSize != S.getLangOpts().ArmSveVectorBits` above I think this is ok as this shouldn't be reachable with `-msve-vector-bits=<bits>` validating the vector size.


================
Comment at: clang/test/Sema/attr-arm-sve-vector-bits.c:80
+  void *sel __attribute__((unused));
+  sel = c ? ss8 : fs8; // expected-error {{incompatible operand types ('svint8_t' (aka '__SVInt8_t') and 'fixed_int8_t' (aka '__SVInt8_t'))}}
+  sel = c ? fs8 : ss8; // expected-error {{incompatible operand types ('fixed_int8_t' (aka '__SVInt8_t') and 'svint8_t' (aka '__SVInt8_t'))}}
----------------
sdesmalen wrote:
> Is this diagnostic produced because of any code-changes in this patch?
> Is this diagnostic produced because of any code-changes in this patch?

It isn't actually, I guess this could have gone in the first patch. Happy to move it if you think it's necessary.


================
Comment at: clang/test/Sema/attr-arm-sve-vector-bits.c:136
+
+struct struct_int8 { fixed_int8_t x, y[5]; };
+struct struct_int16 { fixed_int16_t x, y[5]; };
----------------
sdesmalen wrote:
> nit: Is it necessary to test this for all the types?
> nit: Is it necessary to test this for all the types?

I've reduced the number of types tested here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83551/new/

https://reviews.llvm.org/D83551