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

[Bryan Chan via Phabricator via cfe-commits]

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


================
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>]>;
----------------
david-arm wrote:
> 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!
Thanks for the suggestion. Refactored.


================
Comment at: clang/lib/Headers/CMakeLists.txt:332
+  # Generate arm_sme.h
+  clang_generate_header(-gen-arm-sme-header arm_sme.td arm_sme.h)
   # Generate arm_bf16.h
----------------
bryanpkc wrote:
> sdesmalen wrote:
> > The ACLE specification is still in a draft (ALP) state, which means there may still be subject to significant changes. To avoid users from using this implementation with the expectation that their code is compliant going forward, it would be good to rename the header file to something that makes it very clear this feature is not yet ready to use. I'm thinking of something like `arm_sme_draft_spec_subject_to_change.h`. When the specification goes out of draft, we can rename it to `arm_sme.h`. Could you rename the file for now?
> Would something shorter like `arm_sme_draft.h` or `arm_sme_experimental.h` suffice?
Renamed to `arm_sme_experimental.h`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127910