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: > > > > > 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? > I don't have a good idea of that, I know little about our modules > implementation. Perhaps @ChuanqiXu knows? Gentle ping @ChuanqiXu Do you know if this patch requires any further changes for reading/writing modules? The test I've added seems to work fine. ================ Comment at: clang/include/clang/Basic/Attr.td:2459 +def ArmPreservesZA : TypeAttr, TargetSpecificAttr<TargetAArch64SME> { + let Spellings = [RegularKeyword<"__arm_preserves_za">]; + let Subjects = SubjectList<[HasFunctionProto], ErrorDiag>; ---------------- erichkeane wrote: > So, why are `__arm_shared_za` and `__arm_preserves_za` different tenses, > despite being very similar sounding? Should it be `__arm_shares_za` (or > alternatively, `__arm_preserved_za`)? I think this was mostly for historical reasons. @rsandifo-arm mentioned to me that he was still open to renaming these. 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
