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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
