Page MenuHomePhabricator

[Clang][AArch64] Add SME C intrinsics for load and store
Needs ReviewPublic

Authored by sagarkulkarni19 on Jun 15 2022, 3:44 PM.

Details

Summary

This patch adds support for the following SME ACLE intrinsics:

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

Defined in : https://github.com/ARM-software/acle/pull/188

Currently, the intrinsics are generated using arm_sve.td with
modifications to TableGen/SveEmitter.cpp to generate arm_sme.h

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2022, 3:44 PM
sagarkulkarni19 requested review of this revision.Jun 15 2022, 3:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2022, 3:44 PM

Hi @sagarkulkarni19, this patch will have to wait until the ABI is implemented so that the builtins can be decorated with the attributes. A first patch proposing these attributes is in D127762. LLVM patches that use these attributes to implement the ABI will follow soon. We could add you as reviewer to these patches if you'd like to help us with that?

clang/include/clang/Basic/arm_sve.td
209

Is there value in having both IsSME and IsSMELoadStore?

clang/lib/Basic/Targets/AArch64.cpp
342

When this macro is non-zero, it suggests that the compiler implements the full SME ACLE. That is currently not yet the case, so until then we should leave this macro undefined.

Hi @sagarkulkarni19, this patch will have to wait until the ABI is implemented so that the builtins can be decorated with the attributes. A first patch proposing these attributes is in D127762. LLVM patches that use these attributes to implement the ABI will follow soon. We could add you as reviewer to these patches if you'd like to help us with that?

Hi @sdesmalen, sounds good. I can update this patch accordingly when the attributes patches are in. Sure, please add me as a reviewer to those patches. We are porting much of our downstream C intrinsics implementation to support the ACLE instead and can contribute this effort upstream. I have also created an article on discourse (https://discourse.llvm.org/t/clang-sme-acle-intrinsics/63223) for further discussion on this.

clang/include/clang/Basic/arm_sve.td
209

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.

clang/lib/Basic/Targets/AArch64.cpp
342

Okay makes sense. But until all SME ACLE is implemented, do we need some sort of guard in the meantime while the SME intrinsics that are implemented incrementally?

Matt added a subscriber: Matt.Jun 16 2022, 4:43 PM
sdesmalen added inline comments.Jun 17 2022, 4:29 AM
clang/lib/Basic/Targets/AArch64.cpp
342

The tests need to have manual -D__ARM_FEATURE_SME in the RUN lines. Once the feature is implemented and we add the definition automatically when +sme is specified, we update the tests. See for example D81725 where we did this for SVE.

sagarkulkarni19 edited the summary of this revision. (Show Details)

Updated testcases and also added the vnum variant of the ld1 and st1 intrinsics.

sagarkulkarni19 marked 2 inline comments as done.Jun 24 2022, 4:40 PM
sagarkulkarni19 added inline comments.
clang/lib/Basic/Targets/AArch64.cpp
342

Thanks. That makes sense, now the macro is undefined and I have updated the testcases accordingly.

sagarkulkarni19 marked an inline comment as done.Jun 24 2022, 5:19 PM