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

[Sander de Smalen via Phabricator via cfe-commits]

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