[PATCH] D127910: [Clang][AArch64][SME] Add vector load/store (ld1/st1) intrinsics

Mon Feb 13 08:11:57 PST 2023

dmgreen added inline comments.

================
Comment at: clang/utils/TableGen/SveEmitter.cpp:1477
+
+  OS << "#if !defined(__ARM_FEATURE_SME)\n";
+  OS << "#error \"SME support not enabled\"\n";
----------------
bryanpkc wrote:
> dmgreen wrote:
> > We have been changing how the existing SVE and NEON instrinsics work a little. There are some details in https://reviews.llvm.org/D131064. The short version is it is best to not rely upon preprocessor macros, and instead define the intrinsics so that they can be used if the right target features are available. This allows us to do things like this below, even without a -march that supports sme, and have them callable at runtime under the right situations. We should be doing the same for SME.
> > ```
> > __attribute__((target("+sme")))
> > void sme_func() {
> >   somesmeintrinsics();
> > }
> > ```
> I have refactored the SME intrinsics in a similar fashion. Could you confirm if I did it correctly?
It looks OK - as far as I can see. It might be worth adding a test for it? But otherwise it looks good. Thanks.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127910