Page MenuHomePhabricator

[AArch64][SelectionDAG] CodeGen for Armv8.8/9.3 MOPS

Authored by tyb0807 on Jan 20 2022, 2:30 AM.



New target SDNodes are added: AArch64ISD::MOPS_MEMSET, etc.
Each intrinsic is translated to one of these in SelectionDAGBuilder
via EmitTargetCodeForMOPS.

A custom lowering routine for INTRINSIC_W_CHAIN is added to handle
llvm.aarch64.mops.memset.tag. This takes a separate path from the common
intrinsics but ultimately ends up in the same EmitMOPS().

This is part 4/4 of a series of patches split from to facilitate reviewing.

Patch by Tomas Matheson, Lucas Prates and Son Tuan Vu.

Diff Detail

Event Timeline

tyb0807 created this revision.Jan 20 2022, 2:30 AM
tyb0807 requested review of this revision.Jan 20 2022, 2:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2022, 2:30 AM
dmgreen added inline comments.Jan 20 2022, 9:10 AM

I think that if the values in both sides of the if are currently the same - it's probably worth removing the if. We can always re-add it again if/when we alter these in the future.


-> "So hide SizeWb using MERGE_VALUES."


What does "We don't use it" mean here?


Maybe use != MVT::i8, or add an assert. I guess it will always be an i8 or a i64? Will this only be for Set?

In llvm, singled statements don't need brackets around them and can be dropped.


You can usually remove "hidden" "dso_local" and "local_unnamed_addr", to clean up the test a little. "Function Attrs: " too.

tyb0807 marked 4 inline comments as done.Jan 20 2022, 3:35 PM
tyb0807 added inline comments.

Not sure. I propose to remove it


I'm not sure. For Intrinsic::memcpy and Intrinsic::memmove, from, it seems that the type for SrcOrValue is llvm_anyptr_ty. Am I missing something here?

tyb0807 updated this revision to Diff 401850.Jan 20 2022, 8:42 PM
tyb0807 marked an inline comment as done.

Fix typos and cleanup tests.

dmgreen added inline comments.Jan 24 2022, 3:54 AM

Clang format should put these in order


I'm not sure this is worth adding - if it turns out that the existing costs are fine then it will be left as a FIXME for all time without adding much useful information.


The condition doesn't need getSimpleVT().
I think this should only happen with IsSet, so it's probably worth sinking into the if below. The it might as well be if if (SrcOrValue.getValueType() != MVT::i64)

dmgreen added inline comments.Jan 24 2022, 4:01 AM

This is also creating Machine nodes very early. It may be better to create an ISD node for it instead. (Although I'm not sure how much it will matter - if there are no dag optimizations that happen for the memcpy nodes).

tyb0807 marked 4 inline comments as done.Jan 24 2022, 10:58 AM
tyb0807 updated this revision to Diff 402654.Jan 24 2022, 1:48 PM

Remove FIXME and minor code refactoring

tyb0807 added inline comments.Jan 24 2022, 1:52 PM

Since we are not aware of any DAGCombine pattern allowing to optimize memcpy nodes, I propose to create Machine nodes here instead of defining new ISD nodes for these instructions, then do the ISD nodes -> Machine nodes somewhere else. What do you think?

dmgreen accepted this revision.Jan 25 2022, 2:28 AM

Thanks. LGTM


You can leave this comment in.


It can cause issues here and there. But if we need it, we can change it later.

This revision is now accepted and ready to land.Jan 25 2022, 2:28 AM
tyb0807 updated this revision to Diff 402885.Jan 25 2022, 6:18 AM
tyb0807 marked 3 inline comments as done.

Re-add comment explaining that llvm.mem* intrinsics can be either lowered to
ldr/str sequences or pseudo-instructions

Matt added a subscriber: Matt.Jan 25 2022, 3:12 PM
tyb0807 updated this revision to Diff 404346.Jan 30 2022, 12:04 AM

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

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

FYI getElementType was deprecated in I've got a build with -Wdeprecated -Werror that is complaining about this. Setting aside that that's probably a silly thing to have, it would be good to update this, I think

tyb0807 marked an inline comment as done.Feb 1 2022, 4:41 AM
tyb0807 added inline comments.

This has been temporarily fixed in
This will correctly be fixed in

GMNGeoffrey added inline comments.Feb 1 2022, 9:27 AM