This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Add fixed type lowering for EXTRACT_SUBVECTOR
ClosedPublic

Authored by bsmith on Oct 5 2021, 8:59 AM.

Diff Detail

Event Timeline

bsmith created this revision.Oct 5 2021, 8:59 AM
bsmith published this revision for review.Oct 5 2021, 9:00 AM
bsmith retitled this revision from [AArch64][SVE] Add fixed type lowering for EXTRACT_SUBVECTOR Depends on D111135 to [AArch64][SVE] Add fixed type lowering for EXTRACT_SUBVECTOR.
bsmith edited the summary of this revision. (Show Details)
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2021, 9:01 AM
Matt added a subscriber: Matt.Oct 5 2021, 9:43 AM
bsmith updated this revision to Diff 377489.Oct 6 2021, 3:18 AM
  • Rebase onto dependant patch
paulwalker-arm added inline comments.Oct 7 2021, 10:27 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
10734

Is it worth moving this block after the special cases below? I'm just think if there's a time when useSVEForFixedLengthVectorVT accepts 128bit or smaller vectors.

10742

Can you use NewInVec here as UNDEF could create a false dependency that hurts performance.

16895–16896

To me this looks like we're missing a trivial fold within getNode for either BITCAST or whatever DAG.getAnyExtOrTrunc creates?

bsmith updated this revision to Diff 378158.Oct 8 2021, 3:46 AM
  • Move fixed type lowering after lowering that ends in no-op
  • Don't use undef in lowering
  • Remove splice combine change as it is no longer needed
paulwalker-arm added inline comments.Oct 8 2021, 8:42 AM
llvm/test/CodeGen/AArch64/sve-fixed-length-extract-subvector.ll
62–68

There's a few tests like this that I suspect are not really exercising the extract_subvector logic. Within sve-fixed-length-subvector.ll you'll see I hit a similar issue along with how I worked round it. Essentially for all the tests you want to ensure the call to experimental.vector.extract is within it's own block.

bsmith added inline comments.Oct 11 2021, 8:29 AM
llvm/test/CodeGen/AArch64/sve-fixed-length-extract-subvector.ll
62–68

I'm not actually sure there is anything we can do about this, putting this in it's own block doesn't help. The operand to extract_subvector is not a legal type, hence the legalizer is splitting the operand before we get to try and lower it (which is what we want), which ends up just discarding the extract_subvector.

paulwalker-arm accepted this revision.Oct 11 2021, 8:54 AM
paulwalker-arm added inline comments.
llvm/test/CodeGen/AArch64/sve-fixed-length-extract-subvector.ll
62–68

I see what you mean.

This revision is now accepted and ready to land.Oct 11 2021, 8:54 AM
sdesmalen accepted this revision.Oct 11 2021, 9:31 AM

Nice improvement!

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

I'm not sure if this really matters because it would probably reuse NewInVec anyway, but strictly this could be UNDEF.

bsmith added inline comments.Oct 11 2021, 10:03 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
10752

I think it's probably best to leave this as NewInVec, whilst you could probably get away with UNDEF most of the time, you could in theory create false dependencies as Paul noted previously.