Page MenuHomePhabricator

[AArch64] Support for memset tagged intrinsic
ClosedPublic

Authored by tyb0807 on Jan 20 2022, 12:29 AM.

Details

Summary

This introduces a new ACLE intrinsic for memset tagged
(https://github.com/ARM-software/acle/blob/next-release/main/acle.md#memcpy-family-of-operations-intrinsics---mops).

void *__builtin_arm_mops_memset_tag(void *, int, size_t)

A corresponding LLVM intrinsic is introduced:

i8* llvm.aarch64.mops.memset.tag(i8*, i8, i64)

The types match llvm.memset but the return type is not void.

This is part 1/4 of a series of patches split from
https://reviews.llvm.org/D117405 to facilitate reviewing.

Patch by Tomas Matheson

Diff Detail

Event Timeline

tyb0807 created this revision.Jan 20 2022, 12:29 AM
tyb0807 requested review of this revision.Jan 20 2022, 12:29 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 20 2022, 12:29 AM
tyb0807 updated this revision to Diff 401534.Jan 20 2022, 12:33 AM
tyb0807 edited the summary of this revision. (Show Details)

Update commit message to clarify context.

SjoerdMeijer added inline comments.
clang/lib/Headers/arm_acle.h
734

Why does this also need MTE? I think the ACLE specifies this intrinsic to be available when __ARM_FEATURE_MOPS is defined?

dmgreen added inline comments.Jan 20 2022, 9:29 AM
clang/lib/Headers/arm_acle.h
736

The arguments are better named value and size, etc. That way they only use reserved names and uses won't run into trouble if they #define value 42 before the include.

dmgreen added inline comments.Jan 20 2022, 9:30 AM
clang/lib/Headers/arm_acle.h
736

Oh, I mean __value and __size, etc :)

tyb0807 marked 3 inline comments as done.Jan 20 2022, 3:35 PM
tyb0807 added inline comments.
clang/lib/Headers/arm_acle.h
734

Yes you are right, thanks for spotting this.

736

Yes, thanks for the review

tyb0807 updated this revision to Diff 401851.Jan 20 2022, 9:00 PM
tyb0807 marked 2 inline comments as done.

__ARM_FEATURE_MEMORY_TAGGING not needed to enable __builtin_arm_mops_memset_tag.

Follow variable naming convention.

tyb0807 updated this revision to Diff 401973.Jan 21 2022, 6:38 AM
tyb0807 edited the summary of this revision. (Show Details)

Update reference to ACLE specification

SjoerdMeijer added inline comments.Jan 25 2022, 2:36 AM
clang/test/CodeGen/aarch64-mops.c
4

I forgot if we add negative tests for these things, i.e. check if we error if we don't have +mops or __ARM_FEATURE_MOPS set? I guess so?

tyb0807 marked an inline comment as done.Jan 25 2022, 6:13 AM
tyb0807 added inline comments.
clang/test/CodeGen/aarch64-mops.c
4

Currently this only tests if the support for memset tagged intrinsic is in clang, how to enable the memset tagged intrinsic is implemented (and tested) in a separate patch (https://reviews.llvm.org/D116160). Note how we set __ARM_FEATURE_MOPS manually in line 5.

This test will thus be updated with negative tests in https://reviews.llvm.org/D116160

Matt added a subscriber: Matt.Jan 25 2022, 3:11 PM
This revision is now accepted and ready to land.Jan 26 2022, 2:55 AM
dmgreen added inline comments.Jan 27 2022, 5:04 AM
clang/lib/Headers/arm_acle.h
734

Hmm. These map to SETMG, and those instructions require MTE. It wouldn't make sense to have an intrinsic that cannot be emitted to a valid instruction. I think the spec might be wrong, to be honest.

SjoerdMeijer added inline comments.Jan 27 2022, 7:00 AM
clang/lib/Headers/arm_acle.h
734

Ah yeah, thanks Dave. These are the tag setting variants, I didn't look careful enough, then probably got confused about the ACLE which I agree must be wrong. Would be good to double check that first.

dmgreen added inline comments.Jan 27 2022, 7:03 AM
clang/lib/Headers/arm_acle.h
734

Yep - Apparently it is getting fixed in https://github.com/ARM-software/acle/pull/161 thanks to Lucas

tyb0807 updated this revision to Diff 403686.EditedJan 27 2022, 8:57 AM
tyb0807 marked an inline comment as done.

Update accordingly to change from ACLE specification: __builtin_arm_mops_memset_tag requires _both_ MOPS and MTE features

tyb0807 updated this revision to Diff 403687.Jan 27 2022, 8:58 AM
tyb0807 marked 3 inline comments as done.
tyb0807 edited the summary of this revision. (Show Details)

Update link to the relevant section of ACLE specification

dmgreen accepted this revision.Jan 30 2022, 1:02 PM

Thanks. LGTM

This revision was landed with ongoing or failed builds.Jan 31 2022, 12:49 PM
This revision was automatically updated to reflect the committed changes.