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

[David Sherwood via Phabricator via cfe-commits]

david-arm added a comment.

Hi @bryanpkc, this is looking a lot better now and thanks for addressing the comments! I've not reviewed all of the patch yet, but I do have a few more comments. The most important ones are about performing immediate range checks for the builtins and not declaring the __ARM_FEATURE_SME yet.



================
Comment at: clang/include/clang/Basic/TargetBuiltins.h:312
+  /// Flags to identify the types for overloaded SME builtins.
+  class SMETypeFlags {
+    uint64_t Flags;
----------------
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.


================
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">;
----------------
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.



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


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:438
 
+  if (HasSME)
+    Builder.defineMacro("__ARM_FEATURE_SME", "1");
----------------
Can you remove this please? We can't really set this macro until the SME ABI and ACLE is feature complete.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:9315
+                                      unsigned IntID) {
+  switch (IntID) {
+  case Intrinsic::aarch64_sme_ld1h_horiz:
----------------
I think that instead of this switch statement you should just be able to write something like:

  Ops[3] = EmitSVEPredicateCast(Ops[3],  getSVEVectorForElementType(SVEBuiltinMemEltTy(TypeFlags)))



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:9357
+    Function *StreamingVectorLength =
+        CGM.getIntrinsic(Intrinsic::aarch64_sme_cntsb, {});
+    llvm::Value *StreamingVectorLengthCall =
----------------
I think you can just call

  CGM.getIntrinsic(Intrinsic::aarch64_sme_cntsb)


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:9359
+    llvm::Value *StreamingVectorLengthCall =
+        Builder.CreateCall(StreamingVectorLength, {});
+    llvm::Value *Mulvl =
----------------
Again, I think you can just do

  Builder.CreateCall(StreamingVectorLength)


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:9368
+  NewOps.push_back(EmitTileslice(Ops[2], Ops[1]));
+  Function *F = CGM.getIntrinsic(IntID, {});
+  return Builder.CreateCall(F, NewOps);
----------------
nit: `Function *F = CGM.getIntrinsic(IntID);`


================
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}}
----------------
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.


================
Comment at: clang/utils/TableGen/SveEmitter.cpp:1634
+void SVEEmitter::createSMETypeFlags(raw_ostream &OS) {
+  OS << "#ifdef LLVM_GET_SME_TYPEFLAGS\n";
+  for (auto &KV : FlagTypes)
----------------
If you reuse the existing SVETypeFlags rather than create a new SMETypeFlags then you only need the LLVM_GET_SME_IMMCHECKTYPES bit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127910