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

[David Sherwood via Phabricator via cfe-commits]

david-arm added a comment.

Thanks a lot for making all the changes @bryanpkc - it's looking really good now! I just have a few minor comments/suggestions and then I think it looks good to go.



================
Comment at: clang/include/clang/Basic/arm_sme.td:22
+let TargetGuard = "sme" in {
+  def SVLD1_HOR_ZA8 : MInst<"svld1_hor_za8", "vimiPQ", "c", [IsLoad, IsOverloadNone, IsStreaming, IsSharedZA], MemEltTyDefault, "aarch64_sme_ld1b_horiz", [ImmCheck<0, ImmCheck0_0>, ImmCheck<2, ImmCheck0_15>]>;
+  def SVLD1_HOR_ZA16 : MInst<"svld1_hor_za16", "vimiPQ", "s", [IsLoad, IsOverloadNone, IsStreaming, IsSharedZA], MemEltTyDefault, "aarch64_sme_ld1h_horiz", [ImmCheck<0, ImmCheck0_1>, ImmCheck<2, ImmCheck0_7>]>;
----------------
This is just a suggestion, but you could reduce the lines of code here if you want by creating a multiclass that creates both the horizontal and vertical variants for each size, i.e. something like

  multiclass MyMultiClass<..> {
    def NAME # _H : MInst<...>
    def NAME # _V : MInst<...>
  }

  defm SVLD1_ZA8 : MyMultiClass<...>;

or whatever naming scheme you prefer, and same for the stores. Feel free to ignore this suggestion though if it doesn't help you!


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:438
 
+  if (HasSME)
+    Builder.defineMacro("__ARM_FEATURE_SME", "1");
----------------
bryanpkc wrote:
> 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?
It should have complete support for the SME ABI and ACLE in terms of supporting the C/C++ level attributes as described here  - https://arm-software.github.io/acle/main/acle.html#sme-language-extensions-and-intrinsics. For example, the compiler should support cases where a normal function calls a `arm_streaming` function and sets up the state correctly, etc. You can see @sdesmalen's patch to add the clang-side attributes here D127762. There should also be full support for all of the SME ACLE builtins.


================
Comment at: clang/test/Sema/aarch64-sme-intrinsics/acle_sme_imm.cpp:3
+
+// RUN: %clang_cc1 -D__ARM_FEATURE_SME -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +sme -fsyntax-only -verify -verify-ignore-unexpected=error %s
+// RUN: %clang_cc1 -D__ARM_FEATURE_SME -DSVE_OVERLOADED_FORMS -triple aarch64-none-linux-gnu -target-feature +sve -target-feature +sme -fsyntax-only -verify -verify-ignore-unexpected=error %s
----------------
I think you can remove the `-target-feature +sve` flags from the RUN lines because `+sme` should imply that.


================
Comment at: clang/test/Sema/aarch64-sme-intrinsics/acle_sme_imm.cpp:16
+__attribute__((arm_streaming))
+void test_range_0_0(svbool_t pg, void *ptr) {
+  // expected-error-re at +1 {{argument value 0 is outside the valid range [0, 0]}}
----------------
These tests are great! Thanks for doing this. Could you also add the `_vnum` variants too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127910