This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Summary

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
https://reviews.llvm.org/D117405 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
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
946–947

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.

4093

-> "So hide SizeWb using MERGE_VALUES."

llvm/lib/Target/AArch64/AArch64SelectionDAGInfo.cpp
26

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

63

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.

llvm/test/CodeGen/AArch64/aarch64-mops-consecutive.ll
11

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.
llvm/lib/Target/AArch64/AArch64SelectionDAGInfo.cpp
26

Not sure. I propose to remove it

63

I'm not sure. For Intrinsic::memcpy and Intrinsic::memmove, from Intrinsics.td, 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
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
34

Clang format should put these in order

940

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.

llvm/lib/Target/AArch64/AArch64SelectionDAGInfo.cpp
63

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
llvm/lib/Target/AArch64/AArch64SelectionDAGInfo.cpp
70

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
llvm/lib/Target/AArch64/AArch64SelectionDAGInfo.cpp
70

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

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
939

You can leave this comment in.

llvm/lib/Target/AArch64/AArch64SelectionDAGInfo.cpp
70

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.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
11898

FYI getElementType was deprecated in https://github.com/llvm/llvm-project/commit/184591a. 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.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
11898

This has been temporarily fixed in https://reviews.llvm.org/rG51ed14d22430bed0d191e01f7efeae59b1aee5e0.
This will correctly be fixed in https://reviews.llvm.org/D118681

GMNGeoffrey added inline comments.Feb 1 2022, 9:27 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
11898

Thanks!