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

[Richard Sandiford via Phabricator via cfe-commits]

rsandifo-arm added a comment.

The intent looks good to me, but I'm not in a position to approve.  Very pedantic, sorry, but on the description, I think:

> Calls from non-streaming functions to streaming-functions require the compiler to enable/disable streaming-SVE mode around the call-site.

would be more accurate as:

> Calls from non-streaming **mode** to streaming functions…

since it applies to calls from streaming-compatible functions too.  (Which is what the patch implements.)



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3629
+  "call to a streaming function requires sme">;
+def err_sme_definition_in_non_sme_target : Error<
+  "function executed in streaming-SVE mode or using ZA state, requires sme">;
----------------
Seems like we can easily split this into two and distinguish the cases, since the code that gives the error can always tell which case applies.  I think not having SME for streaming mode is logically more fundamental than not having SME for ZA.

I think `sme` should be SME (if it's referring to the ISA feature) or should be quoted (if it's referring to the feature syntax).


================
Comment at: clang/lib/Sema/SemaChecking.cpp:6631
 
 /// Handles the checks for format strings, non-POD arguments to vararg
 /// functions, NULL arguments passed to non-NULL parameters, and diagnose_if
----------------
I don't know the code well enough to know whether this is the right place to check this.  if it is, though, I suppose the comment needs to be updated too.

I agree the intention looks right from a spec POV.


================
Comment at: clang/test/Sema/aarch64-sme-func-attrs-without-target-feature.cpp:29
+
+void streaming_compatible_def2() {
+  non_streaming_decl(); // OK
----------------
Looks like this was intended to be `__arm_streaming_compatible`.


================
Comment at: llvm/lib/Target/AArch64/AArch64SMEInstrInfo.td:158
+
+  // Rather than only allowing this instruction with '+sme', we also
+  // allow it with '+sve', such that we can still emit the instruction
----------------
I don't think even SVE is required.  A function like:
```
void foo() __arm_streaming_compatible {
   printf ("Hello, world!\n");
}
```
should be OK for base Armv8-A.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157269