david-arm added a comment.
Thanks a lot for making all the changes @bryanpkc - it's looking really good
now! I just have a few minor comments/suggestions and then I think it looks
good to go.
================
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>]>;
----------------
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!
================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:438
+ if (HasSME)
+ Builder.defineMacro("__ARM_FEATURE_SME", "1");
----------------
bryanpkc wrote:
> 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?
It should have complete support for the SME ABI and ACLE in terms of supporting
the C/C++ level attributes as described here -
https://arm-software.github.io/acle/main/acle.html#sme-language-extensions-and-intrinsics.
For example, the compiler should support cases where a normal function calls a
`arm_streaming` function and sets up the state correctly, etc. You can see
@sdesmalen's patch to add the clang-side attributes here D127762. There should
also be full support for all of the SME ACLE builtins.
================
Comment at: clang/test/Sema/aarch64-sme-intrinsics/acle_sme_imm.cpp:3
+
+// RUN: %clang_cc1 -D__ARM_FEATURE_SME -triple aarch64-none-linux-gnu
-target-feature +sve -target-feature +sme -fsyntax-only -verify
-verify-ignore-unexpected=error %s
+// RUN: %clang_cc1 -D__ARM_FEATURE_SME -DSVE_OVERLOADED_FORMS -triple
aarch64-none-linux-gnu -target-feature +sve -target-feature +sme -fsyntax-only
-verify -verify-ignore-unexpected=error %s
----------------
I think you can remove the `-target-feature +sve` flags from the RUN lines
because `+sme` should imply that.
================
Comment at: clang/test/Sema/aarch64-sme-intrinsics/acle_sme_imm.cpp:16
+__attribute__((arm_streaming))
+void test_range_0_0(svbool_t pg, void *ptr) {
+ // expected-error-re@+1 {{argument value 0 is outside the valid range [0,
0]}}
----------------
These tests are great! Thanks for doing this. Could you also add the `_vnum`
variants too?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127910/new/
https://reviews.llvm.org/D127910
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
