bryanpkc marked 9 inline comments as done.
bryanpkc added inline comments.
================
Comment at: clang/include/clang/Basic/TargetBuiltins.h:312
+ /// Flags to identify the types for overloaded SME builtins.
+ class SMETypeFlags {
+ uint64_t Flags;
----------------
david-arm wrote:
> 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.
Thanks for the suggestion. I have removed this class.
================
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">;
----------------
david-arm wrote:
> 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.
>
Done.
================
Comment at: clang/include/clang/Basic/arm_sve_sme_incl.td:126
+// Z: const pointer to uint64_t
+
+class MergeType<int val, string suffix=""> {
----------------
david-arm wrote:
> Please can you add a comment here for the new Prototype modifier you added -
> '%'?
A comment for '%' already exists on line 103.
================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:438
+ if (HasSME)
+ Builder.defineMacro("__ARM_FEATURE_SME", "1");
----------------
david-arm wrote:
> Can you remove this please? We can't really set this macro until the SME ABI
> and ACLE is feature complete.
OK. Could you educate me what else is needed for SME ABI and ACLE to be
feature-complete? How can I help?
================
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}}
----------------
david-arm wrote:
> 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.
Done, thanks for the suggestion. I have also moved these tests into
`clang/test/Sema/arm-sme-intrinsics/`.
================
Comment at: clang/utils/TableGen/SveEmitter.cpp:1477
+
+ OS << "#if !defined(__ARM_FEATURE_SME)\n";
+ OS << "#error \"SME support not enabled\"\n";
----------------
dmgreen wrote:
> We have been changing how the existing SVE and NEON instrinsics work a
> little. There are some details in https://reviews.llvm.org/D131064. The short
> version is it is best to not rely upon preprocessor macros, and instead
> define the intrinsics so that they can be used if the right target features
> are available. This allows us to do things like this below, even without a
> -march that supports sme, and have them callable at runtime under the right
> situations. We should be doing the same for SME.
> ```
> __attribute__((target("+sme")))
> void sme_func() {
> somesmeintrinsics();
> }
> ```
I have refactored the SME intrinsics in a similar fashion. Could you confirm if
I did it correctly?
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
