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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
