rsandifo-arm added a subscriber: sdesmalen.
rsandifo-arm added a comment.
Thanks for the review.
================
Comment at: clang/include/clang/Basic/Attr.td:2427-2430
+def ArmStreaming : TypeAttr, TargetSpecificAttr<TargetAArch64> {
+ let Spellings = [RegularKeyword<"__arm_streaming">];
+ let Documentation = [Undocumented];
+}
----------------
aaron.ballman wrote:
> I'd feel more comfortable switching an existing attribute over to use this
> new functionality instead of introducing a new attribute at the same time.
> (Also, we ask that there be no new undocumented attributes unless there's a
> Very Good Reason to leave it undocumented.)
Could you suggest an attribute that would be suitable? The problem is that, as
far as I know, no existing keyword has parse rules that exactly match the rules
for standard attributes. (That, and the fact that different keywords have
different rules, was one of the main motivations for the patch). So I don't
think it would be possible to adopt the new scheme for existing attributes
without breaking compatiblity (both ways: some things would become valid that
weren't before, and some things that were valid before would become invalid).
E.g.:
==== __declspec
I don't think anyone was suggesting we try to convert this :-) but for
completeness:
```
int __declspec(align(16)) a1; // OK, but standard attribute rules would say
this appertains to the type
int a2 __declspec(); // Not valid, unlike for standard decl attributes
```
==== __forceinline
```
int __forceinline a1() { return 1; } // OK, but standard attribute rules would
say this appertains to the type
int a2 __forceinline() { return 1; } // Not valid, but would be for standard
decl attributes
```
==== Calling convention keywords
These are generally DeclOrType attributes, with attributes sliding from the
decl to the type where necessary.
```
__stdcall int a1(); // OK, but appertains to the decl and relies on sliding
behaviour
int a2 __stdcall(); // Not valid, but is another place to put standard decl
attributes
int a3() __stdcall; // Not valid, but is another place to put standard type
attributes
int (__stdcall a4)(); // OK, but standard attributes aren't allowed in this
position
extern int (*const __stdcall volatile a5) (); // OK, but standard attributes
wouldn't be allowed here
```
==== Nullability keywords
Like the calling-convention keywords, the nullability keywords appertain to
types but are allowed in some (but not all) positions that would normally
appertain to decls:
```
using ptr = int *;
_Nullable ptr a1; // OK, but appertains to the decl and relies on sliding
behaviour
ptr a2 _Nullable; // Not valid, but is another place to put standard decl
attributes
extern int *const _Nullable volatile a3; // OK, but a standard attribute
wouldn't be allowed here
```
The same distinction applies to Microsoft pointer attributes.
On the documentation side: I admit this is one of the awkward things about
using a new keyword to prove the system. The series doesn't actually implement
`__arm_streaming` (@sdesmalen has patches for that, and other SME stuff). So
it didn't seem appropriate to document the attribute until it was actually
implemented. But the implementation, when it comes, will/should have
documentation.
================
Comment at: clang/lib/AST/TypePrinter.cpp:1724-1727
+ if (T->getAttrKind() == attr::ArmStreaming) {
+ OS << "__arm_streaming";
+ return;
+ }
----------------
aaron.ballman wrote:
> This seems like something that tablegen should automatically handle more
> generally for these attributes.
I wondered about trying to use tablegen for TypePrinter::printAttributedAfter,
but decided against it.
RegularKeyword is really a spelling-level classification rather than an
attribute-level classification, and in general, an attribute could have both
GNU and RegularKeyword spellings. In contrast, printAttributedAfter is only
given the attribute kind and the type that results from applying the attribute.
AFAIK, it doesn't have access to the original attribute spelling. This means
that some attribute-specific or type-specific knowledge might be needed to
print the attribute in the best way.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148700/new/
https://reviews.llvm.org/D148700
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
