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

Thu Feb 23 09:31:58 PST 2023

kmclaughlin accepted this revision.
kmclaughlin added a comment.
This revision is now accepted and ready to land.

Thank you for checking and removing EltTypeBool128. I think you have addressed all of the other comments on this patch too, so it looks good to me!

Please can you update the commit message before landing, to change the reference to `arm_sme_experimental.h` with `arm_sme_draft_spec_subject_to_change.h`?

================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:8874
   case SVETypeFlags::EltTyBool64:
+  case SVETypeFlags::EltTyBool128:
     return Builder.getInt1Ty();
----------------
bryanpkc wrote:
> kmclaughlin wrote:
> > Is it necessary to add an `EltTypeBool128`? I think the EmitSVEPredicateCast call in EmitSMELd1St1 is creating a vector based on the memory element type and not the predicate type?
> You are right, `EltTypeBool128` is not used right now; I can remove it. When we started working on the pre-ACLE intrinsics in our downstream compiler, we were planning to add something like `svptrue_b128`, even though the hardware `PTRUE` instruction doesn't support the Q size specifier. It is still not clear to me how the ACLE wants users to create a predicate vector for a call to a `_za128` intrinsic.
I think the reason why `svptrue_b128` wasn't added is because there is no `ptrue.q` instruction as you say, and it is possible to use any of the other forms (.b, .h, etc) with the 128 bit instructions since they only require every 16th lane.
It would be possible for the user to create a 128 bit predicate with either svunpk or svzip of svbool_t with pfalse where necessary. Otherwise using the other forms, e.g `svptrue_b64()`, will achieve the same result and require fewer instructions.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127910