[PATCH] D127762: [Clang][AArch64] Add/implement ACLE keywords for SME.

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen marked 4 inline comments as done.
sdesmalen added inline comments.


================
Comment at: clang/include/clang/AST/Type.h:3940
+    /// on declarations and function pointers.
+    unsigned AArch64SMEAttributes : 8;
+
----------------
erichkeane wrote:
> sdesmalen wrote:
> > erichkeane wrote:
> > > sdesmalen wrote:
> > > > erichkeane wrote:
> > > > > We seem to be missing all of the modules-storage code for these.  Since this is modifying the AST, we need to increment the 'breaking change' AST code, plus add this to the ASTWriter/ASTReader interface.
> > > > > Since this is modifying the AST, we need to increment the 'breaking change' AST code
> > > > Could you give me some pointers on what you expect to see changed here? I understand your point about adding this to the ASTWriter/Reader interfaces for module-storage, but it's not entirely clear what you mean by "increment the breaking change AST code". 
> > > > 
> > > > I see there is also an ASTImporter, I guess this different from the ASTReader?
> > > > 
> > > > Please apologise my ignorance here, I'm not as familiar with the Clang codebase.
> > > See VersionMajor here: https://clang.llvm.org/doxygen/ASTBitCodes_8h_source.html
> > > 
> > > Yes, ASTReader/ASTWriter and ASTImporter are different unfortunately.  I'm not completely sure of the difference, but doing this patch changing the type here without doing those will break modules.
> > So I tried to create some tests for this (see clang/test/AST/ast-dump-sme-attributes.cpp) and realised that the serialization/deserialization works out-of-the-box.
> > 
> > Does that mean all is needed is an increment of the VERSION_MAJOR or VERSION_MINOR number?
> So its not just ast-dump that matters, it is what happens when these functions are exported from a module, and imported from another?  Unless you make changes to those areas, this SME stuff will be lost, since you're making it part of the type.
Hi @erichkeane I've added a test for module export/import and was surprised to see this work without any additional changes. Do you think the added test is correct/sufficient to show that this works?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6322
+
+Only when Clang defines __ARM_FEATURE_SME, the support for this feature is
+considered to be stable and complete.
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > Double-checking: that macro is missing the trailing double underscore, is that expected?
> Still wondering on this.
Sorry I missed that comment earlier. This is indeed intentional, there is no trailing `__` after `__ARM_FEATURE_SME`.
The macro is defined here: https://github.com/ARM-software/acle/blob/main/main/acle.md#scalable-matrix-extension-sme


================
Comment at: clang/test/Sema/aarch64-sme-func-attrs.c:172
+// expected-cpp-note at -4 {{candidate template ignored: could not match 'short (short) __attribute__((arm_streaming))' against 'short (short)'}}
+template short templated<short>(short);
+#endif
----------------
erichkeane wrote:
> What happens with implicit instantiations?  Can you make sure calling these doesn't lose the attribute?  Our implicit instantiation mechanism has an opt-in for a lot of attributes/etc.
> 
> Additionally, can these be overloaded on in C++?  I don't see any tests for this.  Since they are part of the function type, is that deduced properly? (that is, a function pointer passed as template argument, does it pick up the right type, and does it end up as a separate template isntantiation?).
I've added a few tests for these cases!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127762