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