[PATCH] D127762: [Clang][AArch64] Add ACLE attributes for SME.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

So I still don't see this doing the module writing/reading part, which is necessary if you're making this part of the type.

I'd like to see more testing/thought put into how this interfaces with overloads and template resolution.

Finally, and this is something @aaron.ballman probably wants to answer:  Is this sufficiently important that we're willing to take the additional overhead of 8 bits for each function type (plus an extra 8 bits for its prototype?).  Memory wise, this seems a little costly.



================
Comment at: clang/include/clang/AST/Type.h:3940
+    /// on declarations and function pointers.
+    unsigned AArch64SMEAttributes : 8;
+
----------------
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.


================
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
----------------
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?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127762