[PATCH] D127910: [Clang][AArch64] Add SME C intrinsics for load and store

[David Sherwood via Phabricator via cfe-commits]

david-arm added a comment.

Hi @sagarkulkarni19, thank you for working on the ACLE builtins for SME! I've had a look through and I have a few comments, mostly around how the code is structured. It would be good if you could try to separate SVE from SME in this implementation, in the same way that NEON and SVE are distinct. It's possible to do this whilst reusing much of the code in SveEmitter.cpp.



================
Comment at: clang/include/clang/Basic/arm_sve.td:210
+def IsSME                     : FlagType<0x800000000>;
+def IsSMELd1                  : FlagType<0x1000000000>;
+def IsSMESt1                  : FlagType<0x2000000000>;
----------------
We don't need these new flags 'IsSMELd1' and 'IsSMESt1'. Please can you reuse the existing `IsLoad` and `IsStore` flags?


================
Comment at: clang/include/clang/Basic/arm_sve.td:549
 
+def SVLD1_HOR_ZA8 : MInst<"svld1_hor_za8", "vimiPQ", "", [IsOverloadNone, IsSME, IsSMELd1], MemEltTyDefault, "aarch64_sme_ld1b_horiz">;
+def SVLD1_HOR_ZA16 : MInst<"svld1_hor_za16", "vimiPQ", "", [IsOverloadNone, IsSME, IsSMELd1], MemEltTyDefault, "aarch64_sme_ld1h_horiz">;
----------------
SME is really a distinct architecture and so I think it should live in it's own arm_sme.td file in the same way that we have arm_neon.td and arm_sve.td. It's possible to do this and still reuse the SveEmitter.cpp code. If you look at SveEmitter.cpp you'll see these functions:

  void EmitSveHeader(RecordKeeper &Records, raw_ostream &OS) {
    SVEEmitter(Records).createHeader(OS);
  }

  void EmitSveBuiltins(RecordKeeper &Records, raw_ostream &OS) {
    SVEEmitter(Records).createBuiltins(OS);
  }

  void EmitSveBuiltinCG(RecordKeeper &Records, raw_ostream &OS) {
    SVEEmitter(Records).createCodeGenMap(OS);
  }

  void EmitSveRangeChecks(RecordKeeper &Records, raw_ostream &OS) {
    SVEEmitter(Records).createRangeChecks(OS);
  }

It would be good to add similar ones for SME, i.e. `EmitSmeRangeChecks`, etc.


================
Comment at: clang/include/clang/Basic/arm_sve.td:209
 def IsTupleSet                : FlagType<0x400000000>;
+def IsSME                     : FlagType<0x800000000>;
+def IsSMELoadStore            : FlagType<0x1000000000>;
----------------
sagarkulkarni19 wrote:
> sdesmalen wrote:
> > Is there value in having both `IsSME` and `IsSMELoadStore`?
> `IsSME` flag is checked in the SveEmitter.cpp : createSMEHeader(...) to generate arm_sme.h with only the SME intrinsics, whereas `IsSMELoadStore` is used to correctly CodeGen (CGBuiltins.cpp) load and store intrinsics. 
You don't need the `IsSME` flag either because in `CodeGenFunction::EmitAArch64BuiltinExpr` you can do exactly the same thing as SVE, i.e. something like

  if (BuiltinID >= AArch64::FirstSMEBuiltin &&
      BuiltinID <= AArch64::LastSMEBuiltin)
    return EmitAArch64SMEBuiltinExpr(BuiltinID, E);


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:9230
     return EmitSVEMaskedStore(E, Ops, Builtin->LLVMIntrinsic);
+  else if (TypeFlags.isSMELd1() || TypeFlags.isSMESt1())
+    return EmitSMELd1St1(TypeFlags, Ops, Builtin->LLVMIntrinsic);
----------------
I would prefer this to be handled inside it's own `EmitAArch64SMEBuiltinExpr`, since it's a separate architecture.


================
Comment at: clang/utils/TableGen/SveEmitter.cpp:867
+    if (this->Flags & Emitter.getEnumValueForFlag("IsSMELd1"))
+      this->SMEAttributes = "arm_streaming, arm_shared_za";
+    else if (this->Flags & Emitter.getEnumValueForFlag("IsSMESt1"))
----------------
I would prefer this to be done more precisely via separate attribute flags, i.e. in the .td file decorate each ACLE builtin with something like `IsShared`, `IsStreaming`, `IsStreamingCompatible`, etc. otherwise you'll end up needing loads of flags for all different possible combinations. That way you can do:

  if (this->Flags & Emitter.getEnumValueForFlag("IsStreaming"))
    this->SMEAttributes += "arm_streaming";

etc.


================
Comment at: clang/utils/TableGen/SveEmitter.cpp:1042
 
+  bool SMEFlag = Flags & getEnumValueForFlag("IsSME");
+  if (SMEFlag != IsSME)
----------------
If you move the builtins to their own file in arm_sme.td and emit the records using interfaces like EmitSmeBuiltins, etc. then you already know they are SME builtins and so don't need the flag.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127910