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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
