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

[Bryan Chan via Phabricator via cfe-commits]

bryanpkc marked 9 inline comments as done.
bryanpkc added inline comments.


================
Comment at: clang/include/clang/Basic/TargetBuiltins.h:312
+  /// Flags to identify the types for overloaded SME builtins.
+  class SMETypeFlags {
+    uint64_t Flags;
----------------
david-arm wrote:
> I actually don't think you need to add this class - we should be able to just reuse the existing SVETypeFlags structure. I think this is fine because you've commonised the flags between SME and SVE.
Thanks for the suggestion. I have removed this class.


================
Comment at: clang/include/clang/Basic/arm_sme.td:21
+
+def SVLD1_HOR_ZA8 : MInst<"svld1_hor_za8", "vimiPQ", "c", [IsLoad, IsOverloadNone, IsStreaming, IsSharedZA], MemEltTyDefault, "aarch64_sme_ld1b_horiz">;
+def SVLD1_HOR_ZA16 : MInst<"svld1_hor_za16", "vimiPQ", "s", [IsLoad, IsOverloadNone, IsStreaming, IsSharedZA], MemEltTyDefault, "aarch64_sme_ld1h_horiz">;
----------------
david-arm wrote:
> I think all the load and store instructions need immediate checks for the tile and slice_offset here such as:
> 
> [ImmCheck<0, ImmCheck0>, ImmCheck<2, ImmCheck0_15>]
> 
> for SVLD1_HOR_ZA8 and the others. It's mentioned in the ACLE - https://arm-software.github.io/acle/main/acle.html#sme-language-extensions-and-intrinsics:
> 
>   15.4.3.1 Common Rules
> 
>   ...
>   Every argument named tile, slice_offset or tile_mask must be an integer constant expression in the range of the underlying instruction.
> 
Done.


================
Comment at: clang/include/clang/Basic/arm_sve_sme_incl.td:126
+// Z: const pointer to uint64_t
+
+class MergeType<int val, string suffix=""> {
----------------
david-arm wrote:
> Please can you add a comment here for the new Prototype modifier you added - '%'?
A comment for '%' already exists on line 103.


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:438
 
+  if (HasSME)
+    Builder.defineMacro("__ARM_FEATURE_SME", "1");
----------------
david-arm wrote:
> Can you remove this please? We can't really set this macro until the SME ABI and ACLE is feature complete.
OK. Could you educate me what else is needed for SME ABI and ACLE to be feature-complete? How can I help?


================
Comment at: clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_int_const_expr_error.c:5
+
+__attribute__((arm_streaming)) void test_svld1_hor_za8(uint64_t tile, uint32_t slice_base, uint64_t slice_offset, svbool_t pg, void *ptr) {
+  svld1_hor_za8(tile, slice_base, 0, pg, ptr);          // expected-error {{argument to 'svld1_hor_za8' must be a constant integer}}
----------------
david-arm wrote:
> Once you've added the immediate range checks for the loads and stores it would be good add checks here for immediates outside the range for each instruction.
Done, thanks for the suggestion. I have also moved these tests into `clang/test/Sema/arm-sme-intrinsics/`.


================
Comment at: clang/utils/TableGen/SveEmitter.cpp:1477
+
+  OS << "#if !defined(__ARM_FEATURE_SME)\n";
+  OS << "#error \"SME support not enabled\"\n";
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127910