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