This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SME2] Add CodeGen support for target("aarch64.svcount").
ClosedPublic

Authored by sdesmalen on Oct 27 2022, 10:01 AM.

Details

Summary

This patch adds AArch64 CodeGen support such that the type can be passed
and returned to/from functions, and also adds support to use this type in
load/store operations and PHI nodes.

Diff Detail

Event Timeline

sdesmalen created this revision.Oct 27 2022, 10:01 AM
sdesmalen requested review of this revision.Oct 27 2022, 10:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2022, 10:01 AM
sdesmalen updated this revision to Diff 496100.Feb 9 2023, 5:43 AM
sdesmalen retitled this revision from [AArch64][SME2] Add rudimentary LLVM support for aarch64_svcount. to [AArch64][SME2] Add CodeGen support for target("aarch64.svcount")..
sdesmalen edited the summary of this revision. (Show Details)

Rebased patch to use the target("aarch64.svcount") type.

My class PPRClass related comment is possibly problematic. My preference is to have a clean split between the two predicate types but I realise this is the start of the journey so if making the change results is too much churn then I'm happy to defer it to a refactoring exercise once the initial support has settled. That said, I'd still prefer to see how custom lowering works out rather than duplicating isel patterns.

I'm just going to throw this out there without giving it much thought but what about enabling the ability to ISD::BITCAST between svcount and nxv16i1? I think this better reflects what's going on compared to using reinterpret_cast. It also means we might get "free" operation legalisation by using setOperationPromotedToType.

llvm/include/llvm/Support/MachineValueType.h
407–412

This is more extensible but would return SimpleTy == MVT::aarch64svcount; be so bad?

llvm/lib/CodeGen/CodeGenPrepare.cpp
7699

Do you think it's worth Type having an isScalable() function? if this will be a common idiom.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
17892–17893

What is it about this combine that's problematic? Can you add a code comment for the rational.

Is it specially related to scalable target types or just target types in general?

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
6477–6478

Is this type specific or is it worth adding VT.isScalable()?

9304–9316

Can this be simplified to:

REINTERPRET_CAST(SELECT(CCVal, REINTERPRET_CAST(TVAL), REINTERPRET_CAST(FVAL)))

Then we'll renter this function and reuse the existing predicate lowering code. Perhaps you tried this and it didn't work out?

llvm/lib/Target/AArch64/AArch64RegisterInfo.td
894

Would it be overkill for aarch64svcount to have its own register class? I'm thinking it'll be nice to have isel protect us from incorrectly mixing predicate types and instructions. I'm also not sure if things like SDTCisSameNumEltsAs will cause is trouble, although I guess you've not hit anything so far.

The sequence below is using the non-count naming. Does this matter?

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
2778

Can the loads and stores (well anything that isn't a native svcount instruction) be done during lower?

sdesmalen updated this revision to Diff 499206.Feb 21 2023, 9:16 AM
sdesmalen marked 7 inline comments as done.
  • Added EVT::isScalableVT() and refactored code to use it
  • Use BITCAST instead of REINTERPRET_CAST
  • Custom lower load/store of svcount to use nxv16i1
  • Simplified lowering code in LowerSELECT
  • Replaced the blanket bailout in visitLOAD/visitSTORE in DAGCombiner with more specific bailouts

Thanks for the review @paulwalker-arm, I think I've addressed most of your comments, with the exception of giving predicate-as-counter its own register class, as that one probably makes more sense to pull out into a separate patch.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
17892–17893

This bailout was a bit too rigorous and I've removed it in favour of more specific bailouts.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
9304–9316

Thanks for the suggestion, using SELECT worked fine.

llvm/lib/Target/AArch64/AArch64RegisterInfo.td
894

I tried to give aarch64svcount it's own register class, but this requires changes (to add a new register class, etc) that complicate this patch quite a bit. I can follow this up in a separate patch.

The sequence below is using the non-count naming. Does this matter?

No that's fine, because it uses the same P registers.

paulwalker-arm added inline comments.Feb 22 2023, 6:09 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
420–421

I've tried and now there is isel for ISD::BITCAST this can be

setOperationPromotedToType(ISD::LOAD, MVT::aarch64svcount, MVT::nxv16i1);
setOperationPromotedToType(ISD::STORE, MVT::aarch64svcount, MVT::nxv16i1);
422–423

Do code generation tests for these exist?

llvm/lib/Target/AArch64/AArch64RegisterInfo.td
894

Thanks for the investigation. Sure, having this as a follow on patch works for me.

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
2516

Does PNR exist? or perhaps it's a typo? When experimenting with setOperationPromotedToType I hit build errors and had to change these entries.

sdesmalen updated this revision to Diff 501191.Feb 28 2023, 9:47 AM
sdesmalen marked 4 inline comments as done.
  • Use PromoteToType for svcount_t -> nxv16i1
  • PNR -> PPR in one of the patterns
  • Added tests for select
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
420–421

I'm surprised this worked, as the term 'Promote' makes me think it only works on promoted (element) types, not a bitcast. I've removed the custom lowering code now.

422–423

Thanks for the reminder, I previously added them as part of D136863. I've added them to this patch now.

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
2516

Something probably went wrong with splitting up the patches after I tried adding the predicate-as-counter register class, fixed now.

paulwalker-arm accepted this revision.Mar 1 2023, 9:09 AM

A couple of minor issues but otherwise looks good.

llvm/include/llvm/CodeGen/ValueTypes.h
125

Can you use isZero() here?

llvm/test/CodeGen/AArch64/sme-aarch64-svcount.ll
169–173

Commented out test.

This revision is now accepted and ready to land.Mar 1 2023, 9:09 AM
This revision was landed with ongoing or failed builds.Mar 2 2023, 4:09 AM
This revision was automatically updated to reflect the committed changes.
sdesmalen marked 2 inline comments as done.