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