[Clang][AArch64][SME] Add vector load/store (ld1/st1) intrinsics

[bryanpkc]

Summary

[Clang][AArch64][SME] Add vector load/store (ld1/st1) intrinsics

This patch adds support for the following SME ACLE intrinsics (as defined
in https://arm-software.github.io/acle/main/acle.html):

• svld1_hor_za8 // also for _za16, _za32, _za64 and _za128
• svld1_hor_vnum_za8 // also for _za16, _za32, _za64 and _za128
• svld1_ver_za8 // also for _za16, _za32, _za64 and _za128
• svld1_ver_vnum_za8 // also for _za16, _za32, _za64 and _za128
• svst1_hor_za8 // also for _za16, _za32, _za64 and _za128
• svst1_hor_vnum_za8 // also for _za16, _za32, _za64 and _za128
• svst1_ver_za8 // also for _za16, _za32, _za64 and _za128
• svst1_ver_vnum_za8 // also for _za16, _za32, _za64 and _za128

SveEmitter.cpp is extended to generate arm_sme.h (currently named
arm_sme_draft_spec_subject_to_change.h) and other SME definitions from
arm_sme.td, which is modeled after arm_sve.td. Common TableGen definitions
are moved into arm_sve_sme_incl.td.

Co-authored-by: Sagar Kulkarni <[e-mail]>

[bryanpkc]

Updated the patch to define the __ARM_FEATURE_SME macro when the feature is enabled.

[bryanpkc]

Minor clean-up. Sorry for the noise.

[david-arm]

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.

clang/include/clang/Basic/TargetBuiltins.h

316

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.

clang/include/clang/Basic/arm_sme.td

22

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.

clang/include/clang/Basic/arm_sve_sme_incl.td

127

Please can you add a comment here for the new Prototype modifier you added - '%'?

clang/lib/Basic/Targets/AArch64.cpp

506

Can you remove this please? We can't really set this macro until the SME ABI and ACLE is feature complete.

clang/lib/CodeGen/CGBuiltin.cpp

9407

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)))

9449

I think you can just call

CGM.getIntrinsic(Intrinsic::aarch64_sme_cntsb)

9451

Again, I think you can just do

Builder.CreateCall(StreamingVectorLength)

9460

nit: Function *F = CGM.getIntrinsic(IntID);

clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_int_const_expr_error.c

5 ↗

(On Diff #492586)

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.

clang/utils/TableGen/SveEmitter.cpp

1590

If you reuse the existing SVETypeFlags rather than create a new SMETypeFlags then you only need the LLVM_GET_SME_IMMCHECKTYPES bit.

[dmgreen]

clang/utils/TableGen/SveEmitter.cpp

1433

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();
}

[bryanpkc]

clang/include/clang/Basic/TargetBuiltins.h

316

Thanks for the suggestion. I have removed this class.

clang/include/clang/Basic/arm_sme.td

22

Done.

clang/include/clang/Basic/arm_sve_sme_incl.td

127

A comment for '%' already exists on line 103.

clang/lib/Basic/Targets/AArch64.cpp

506

OK. Could you educate me what else is needed for SME ABI and ACLE to be feature-complete? How can I help?

clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_int_const_expr_error.c

5 ↗

(On Diff #492586)

Done, thanks for the suggestion. I have also moved these tests into clang/test/Sema/arm-sme-intrinsics/.

clang/utils/TableGen/SveEmitter.cpp

1433

I have refactored the SME intrinsics in a similar fashion. Could you confirm if I did it correctly?

[bryanpkc]

Rebased on trunk and addressed review comments.

[david-arm]

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.

clang/include/clang/Basic/arm_sme.td

23

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!

clang/lib/Basic/Targets/AArch64.cpp

506

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.

clang/test/Sema/aarch64-sme-intrinsics/acle_sme_imm.cpp

4

I think you can remove the -target-feature +sve flags from the RUN lines because +sme should imply that.

17

These tests are great! Thanks for doing this. Could you also add the _vnum variants too?

[sdesmalen]

clang/lib/Headers/CMakeLists.txt

344

The ACLE specification is still in a draft (ALP) state, which means there may still be subject to significant changes. To avoid users from using this implementation with the expectation that their code is compliant going forward, it would be good to rename the header file to something that makes it very clear this feature is not yet ready to use. I'm thinking of something like arm_sme_draft_spec_subject_to_change.h. When the specification goes out of draft, we can rename it to arm_sme.h. Could you rename the file for now?

[bryanpkc]

clang/lib/Headers/CMakeLists.txt

344

Would something shorter like arm_sme_draft.h or arm_sme_experimental.h suffice?

[bryanpkc]

Addressed review comments.

[bryanpkc]

clang/include/clang/Basic/arm_sme.td

23

Thanks for the suggestion. Refactored.

clang/lib/Headers/CMakeLists.txt

344

Renamed to arm_sme_experimental.h.

[bryanpkc]

Fixed minor bugs in the previous upload.

[dmgreen]

clang/utils/TableGen/SveEmitter.cpp

1433

It looks OK - as far as I can see. It might be worth adding a test for it? But otherwise it looks good. Thanks.

[sdesmalen]

clang/lib/Headers/CMakeLists.txt

344

While arm_sme_experimental.h is indeed shorter, it should be unequivocally clear to the user that they shouldn't rely on the function prototypes defined in this header file yet because the specification itself is not finalised. I think that adding the draft_spec_subject_to_change to the name makes that more clear than experimental, as the latter might suggest that the support is not yet entirely stable or complete. There probably isn't that much to gain from making the user experience better by using a shorter name, if the whole point is to prevent people from using it for any production code. So from that perspective, I still have a slight preference for arm_sme_draft_spec_subject_to_change.h.

[bryanpkc]

Addressed review comments.

[bryanpkc]

clang/lib/Headers/CMakeLists.txt

344

Renamed as suggested.

clang/test/Sema/aarch64-sme-intrinsics/acle_sme_imm.cpp

4

If I understand llvm/lib/Target/AArch64/AArch64.td and clang/lib/Basic/Targets/AArch64.cpp correctly, -target-feature +sme does not imply -target-feature +sve. I can remove the -D__ARM_FEATURE_SME though.

On the other hand, -march=armv8+sme will imply -target-feature +sve, which I have implemented in D142702.

clang/utils/TableGen/SveEmitter.cpp

1433

Added a test case.

[kmclaughlin]

clang/lib/CodeGen/CGBuiltin.cpp

8934

Is it necessary to add an EltTypeBool128? I think the EmitSVEPredicateCast call in EmitSMELd1St1 is creating a vector based on the memory element type and not the predicate type?

[bryanpkc]

clang/lib/CodeGen/CGBuiltin.cpp

8934

You are right, EltTypeBool128 is not used right now; I can remove it. When we started working on the pre-ACLE intrinsics in our downstream compiler, we were planning to add something like svptrue_b128, even though the hardware PTRUE instruction doesn't support the Q size specifier. It is still not clear to me how the ACLE wants users to create a predicate vector for a call to a _za128 intrinsic.

[kmclaughlin]

Thank you for checking and removing EltTypeBool128. I think you have addressed all of the other comments on this patch too, so it looks good to me!

Please can you update the commit message before landing, to change the reference to arm_sme_experimental.h with arm_sme_draft_spec_subject_to_change.h?

clang/lib/CodeGen/CGBuiltin.cpp

8934

I think the reason why svptrue_b128 wasn't added is because there is no ptrue.q instruction as you say, and it is possible to use any of the other forms (.b, .h, etc) with the 128 bit instructions since they only require every 16th lane.
It would be possible for the user to create a 128 bit predicate with either svunpk or svzip of svbool_t with pfalse where necessary. Otherwise using the other forms, e.g svptrue_b64(), will achieve the same result and require fewer instructions.

[bryanpkc]

Removed EltTypeBool128 as suggested.

[bryanpkc]

Moved test case acle_target_sme.c into clang/test/Sema/aarch64-sme-intrinsics/.

[bryanpkc]

Fix some bugs in the range checks of immediate operands.

[sdesmalen]

clang/lib/Basic/Targets/AArch64.cpp

732–733

Why did you remove this?

clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_ld1.c

17

Hi @bryanpkc, would you be happy to remove the dependence on the attributes patch for now, so that we can move forward to review/land this patch series?

I'm thinking of doing something like:

// RUN: %clang_cc1 -DDISABLE_SME_ATTRIBUTES ....
...

#ifdef DISABLE_SME_ATTRIBUTES
#define ARM_STREAMING_ATTR
#else
#define ARM_STREAMING_ATTR __attribute__((arm_streaming))
#endif

...

ARM_STREAMING_ATTR void test_svld1_hor_za16(uint32_t slice_base, svbool_t pg, const void *ptr) {
  svld1_hor_za16(0, slice_base, 0, pg, ptr);
  svld1_hor_za16(1, slice_base, 7, pg, ptr);
}

With that the tests all pass, and when the attribute patches have landed we can just remove the -DDISABLE_SME_ATTRIBUTES from the RUN lines.

[bryanpkc]

clang/lib/Basic/Targets/AArch64.cpp

732–733

The Feature == "+sme" case is also handled below on line 782. Perhaps we should delete this block and add HasFullFP16 below.

clang/test/CodeGen/aarch64-sme-intrinsics/acle_sme_ld1.c

17

Sounds good, I will give that a shot. If we can unblock this patch, I will also rebase and update the rest of the patch series. Thanks!

[bryanpkc]

@sdesmalen How does the latest version look to you? If you approve, I will land this and finish updating the rest of the patch series. One of the pre-merge builds failed for an unrelated reason, but other jobs were successful.

[sdesmalen]

Thanks @bryanpkc this patch looks good to me now. I'll make some time to review the other patches in the series as well after you update them.