[PATCH] D157269: [Clang][AArch64] Diagnostics for SME attributes when target doesn't have 'sme'

[Paul Walker via Phabricator via cfe-commits]

paulwalker-arm added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:6762
+        Context.getFunctionFeatureMap(CallerFeatureMap, CallerFD);
+        if (!CallerFeatureMap.count("sme"))
+          Diag(Loc, diag::err_sme_call_in_non_sme_target);
----------------
`contains("sme")` seems more appropriate here?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:12159
+      Context.getFunctionFeatureMap(FeatureMap, NewFD);
+      if (!FeatureMap.count("sme")) {
+        if (UsesSM)
----------------
As above.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:12163
+               diag::err_sme_definition_using_sm_in_non_sme_target);
+        else if (UsesZA)
+          Diag(NewFD->getLocation(),
----------------
Can this be just `else` given by this point I believe you know `UsesZA` has to be true.


================
Comment at: clang/test/Sema/aarch64-sme-func-attrs-without-target-feature.cpp:1
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve -fsyntax-only -verify %s
+
----------------
Does the test require SVE to be enabled?


================
Comment at: clang/test/Sema/aarch64-sme-func-attrs-without-target-feature.cpp:3
+
+// This test is testing the diagnostic that Clang emits when compiling without '+sme'.
+
----------------
diagnostics


================
Comment at: clang/test/Sema/aarch64-sme-func-attrs-without-target-feature.cpp:13-17
+__attribute__((target("sme"))) void streaming_compatible_def_sme_attr() __arm_streaming_compatible {} // OK
+__attribute__((target("sme"))) void streaming_def_sme_attr() __arm_streaming { } // OK
+__attribute__((target("sme"))) void shared_za_def_sme_attr() __arm_shared_za { } // OK
+__arm_new_za __attribute__((target("sme"))) void new_za_def_sme_attr() {} // OK
+__arm_locally_streaming __attribute__((target("sme"))) void locally_streaming_def_sme_attr() {} // OK
----------------
Is it worth including tests where "sme2" is used? or are we already comfortable feature inheritance is well tested?


================
Comment at: llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td:144-163
 // It's tricky to using the existing pstate operand defined in
 // AArch64SystemOperands.td since it only encodes 5 bits including op1;op2,
 // when these fields are also encoded in CRm[3:1].
 def MSRpstatesvcrImm1
   : PstateWriteSimple<(ins svcr_op:$pstatefield, timm0_1:$imm), "msr",
                       "\t$pstatefield, $imm">,
     Sched<[WriteSys]> {
----------------
Doesn't this class belong in SMEInstrFormats.td, then you'll not need to override `Predicates`?


================
Comment at: llvm/test/CodeGen/AArch64/sme-call-streaming-compatible-to-normal-fn-wihout-sme-attr.ll:4
+
+; This that the following code can be compiled without +sme, because if the
+; call is not entered in streaming-SVE mode at runtime, the codepath leading
----------------
Verify the...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157269/new/

https://reviews.llvm.org/D157269