aaron.ballman added a comment. > This patch adds all the language-level function attributes defined in: > > https://github.com/ARM-software/acle/pull/188
This pull request has not yet been merged, so what are the chances that it gets rejected or altered? > LLVM support for these attributes will follow later, so at the moment these > attributes are non-functional. FWIW, I would rather we land the attributes once they're functional even if we do a bunch of the review work up front. While rare, we have run into the situation where attributes get added with intentions to make them useful later but then something comes up and we're left with half the implementation exposed to users. ================ Comment at: clang/include/clang/AST/Type.h:3806-3811 + SME_NormalFunction = 0, + SME_PStateSMEnabledMask = 1, + SME_PStateSMCompatibleMask = 2, + SME_PStateZANewMask = 4, + SME_PStateZASharedMask = 8, + SME_PStateZAPreservedMask = 16, ---------------- Also, would it be better to use a bitmask enumeration here? https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/BitmaskEnum.h ================ Comment at: clang/include/clang/AST/Type.h:4008 bool HasTrailingReturn : 1; + unsigned AArch64SMEAttributes : 8; Qualifiers TypeQuals; ---------------- So functions without prototypes cannot have any of these attributes? ================ Comment at: clang/include/clang/Basic/Attr.td:2322 +def ArmStreamingCompatible : DeclOrTypeAttr, TargetSpecificAttr<TargetAArch64> { + let Spellings = [Clang<"arm_streaming_compatible">]; ---------------- `DeclOrTypeAttr` is only intended to be used for attributes which were historically a declaration attribute but are semantically type attributes (like calling conventions). It seems to me that each of these is purely a type attribute and we shouldn't allow them to be written in the declaration position. WDYT? ================ Comment at: clang/include/clang/Basic/Attr.td:2325 + let Subjects = SubjectList<[FunctionLike], ErrorDiag>; + let Documentation = [Undocumented]; +} ---------------- No new, undocumented attributes (same goes for the rest of the additions); please add some docs to AttrDocs.td (it's okay to lightly document and point to a canonical resource for more details). ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8091-8103 + // Handle attributes that are invalid without SME + switch (A.getKind()) { + case ParsedAttr::AT_ArmStreaming: + case ParsedAttr::AT_ArmLocallyStreaming: + case ParsedAttr::AT_ArmSharedZA: + case ParsedAttr::AT_ArmPreservesZA: + case ParsedAttr::AT_ArmNewZA: ---------------- I think this should be handled declaratively in Attr.td by defining a new `TargetArch` that checks this property, similar to: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Attr.td#L387 ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8109-8112 + // Handle mutually exclusive SME function attributes: + // - arm_streaming & arm_streaming_compatible + // - arm_new_za & arm_preserves_za + // - arm_new_za & arm_shared_za ---------------- This should be handled declaratively in Attr.td, like this: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Attr.td#L1403 ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9078-9085 + case ParsedAttr::AT_ArmStreaming: + case ParsedAttr::AT_ArmStreamingCompatible: + case ParsedAttr::AT_ArmLocallyStreaming: + case ParsedAttr::AT_ArmSharedZA: + case ParsedAttr::AT_ArmPreservesZA: + case ParsedAttr::AT_ArmNewZA: + handleSMEAttrs(S, D, AL); ---------------- If the attributes are no longer declaration attributes, then all of this can go away because it needs to be handled from SemaType.cpp instead. ================ Comment at: clang/lib/Sema/SemaType.cpp:7669-7672 + const FunctionProtoType *FnTy = unwrapped.get()->getAs<FunctionProtoType>(); + if (!FnTy) { + // SME ACLE attributes are not supported on K&R-style unprototyped C + // functions. ---------------- Ah, I think the `Subjects` list in Attr.td is incorrect then; it seems like you want to use `HasFunctionProto` instead of `FunctionLike` (or something more similar to `HasFunctionProto` if you really don't want to support ObjC methods or blocks). ================ Comment at: clang/lib/Sema/SemaType.cpp:7676 + return false; + } + ---------------- Unfortunately, type attributes do not yet have any of the automated checking generated by tablegen, so there are no calls to `Sema::checkCommonAttributeFeatures()`, which means you have to manually check things like mutual exclusions, not accepting arguments, etc. ================ Comment at: clang/test/Sema/aarch64-sme-attrs-no-sme.c:1 +// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -fallow-half-arguments-and-returns -fsyntax-only -verify %s + ---------------- I assume that option wasn't necessary for the test? ================ Comment at: clang/test/Sema/aarch64-sme-attrs-no-sme.c:3 + +extern int normal_callee(); + ---------------- ================ Comment at: clang/test/Sema/aarch64-sme-attrs-on-x86.c:1 +// RUN: %clang_cc1 -triple x86_64-none-linux-gnu -target-feature +sme -fallow-half-arguments-and-returns -fsyntax-only -verify %s + ---------------- I think this RUN line should be added to the previous test since the test contents are the same and it's just the diagnostic behavior being tested. You can use `-verify=foo` and `foo-warning {{}}`/`foo-error {{}}` to differentiate between the diagnostics if you need to. ================ Comment at: clang/test/Sema/aarch64-sme-func-attrs.c:1 +// RUN: %clang_cc1 -D__ARM_FEATURE_SME=1 -D__ARM_FEATURE_LOCALLY_STREAMING=1 \ +// RUN: -triple aarch64-none-linux-gnu -target-feature +sme -fsyntax-only -verify %s ---------------- Why are these defines needed? ================ Comment at: clang/test/Sema/aarch64-sme-func-attrs.c:89 +// expected-note@+2 {{conflicting attribute is here}} +__attribute__((arm_preserves_za, arm_new_za)) void preserves_new_za(void); + ---------------- If you keep the attribute as a declaration attribute, you're also missing tests like: ``` __attribute__((arm_new_za)) void func(void); __attribute__((arm_preserves_za)) void func(void) {} ``` ================ Comment at: clang/test/Sema/aarch64-sme-func-attrs.c:107 +typedef __attribute__((arm_new_za, arm_shared_za)) void (*fptrty9) (void); +fptrty9 invalid_shared_za_func() { return shared_za_ptr_invalid; } + ---------------- These are all being used as declaration attributes, I think you also need a test like: ``` void (*fp)(void) [[clang::arm_new_za]] [[clang::arm_preserves_za]]; ``` ================ Comment at: clang/test/Sema/aarch64-sme-func-attrs.c:167 +// expected-warning@+1 {{incompatible function pointer types returning 'pz_ptrty' (aka 'void (*)(void) __attribute__((arm_preserves_za))') from a function with result type 'n_ptrty' (aka 'void (*)(void)')}} +n_ptrty return_invalid_fptr_normal_preserves_za(pz_ptrty f) { return f; } ---------------- You're also missing test coverage for trying to apply the attributes to something other than a function and providing arguments to the attribute. Also, you're missing significant test coverage for C++ where the type system effects are far more interesting. For example, tests that demonstrate you can overload based on the presence of the attribute, tests for template specialization behavior, adding the attribute to a lambda and calling it, etc. There's also some interesting questions there like "can you specify these attributes on a coroutine function?". 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
