Page MenuHomePhabricator

[AArch64][SVE] Lower index_vector to step_vector
ClosedPublic

Authored by junparser on Mon, Apr 19, 10:39 PM.

Details

Summary

As discussed in D100107, this patch first convert index_vector to
step_vector, and convert step_vector back to index_vector after LegalizeDAG.

Diff Detail

Event Timeline

junparser created this revision.Mon, Apr 19, 10:39 PM
junparser requested review of this revision.Mon, Apr 19, 10:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Apr 19, 10:39 PM
Matt added a subscriber: Matt.Sun, Apr 25, 6:52 AM
paulwalker-arm added inline comments.Tue, Apr 27, 5:07 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
13658

You should be able to use Op2 directly and thus remove the need for the SPLAT_VECTOR and MUL.

13665

Is this a phased the rollout? As in I'm assuming that this will ultimately be expressed in terms of STEP_VECTOR as well.

15461–15462

This is already a requirement for the node and asserted with getNode() so doesn't need to be duplicated here.

junparser added inline comments.Tue, Apr 27, 5:20 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
13658

Sounds reasonable to me, update later.

13665

For non-constant op, we need lower index_vector into mul(splat(op2) step_vector()) + splat(base), mul may be lower to shl, even combine as mla with add which may confused the codegen. I'm not sure whether it is worth to deal this.

junparser updated this revision to Diff 341038.Tue, Apr 27, 5:33 PM

Remove assertion.

junparser updated this revision to Diff 341106.Wed, Apr 28, 2:27 AM

Address comments.

Assuming you agree I think you can just delete a bit of code and then the patch is good to go.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
13653–13663

With your previous DAGCombine patches in mind I figured I'd take this patch for a test drive and I believe there's no longer need for this specialisation because the generic block (i.e. the else case) lowers to the expected output (or at least running this patches version of sve-intrinsics-index.ll reports a pass).

15451–15453

FYI: I imagine we're only a few isel patterns away from not need this, but there's no need to hold up this patch for it.

junparser added inline comments.Thu, Apr 29, 6:11 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
13653–13663

Yes, previous DAGCombine patches can handle this. Then I'll remove the if part.

junparser added inline comments.Thu, Apr 29, 6:38 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
15451–15453

Yep, We can remove index_vector node finally

junparser updated this revision to Diff 341730.Thu, Apr 29, 6:38 PM

Address comment.

paulwalker-arm accepted this revision.Fri, Apr 30, 3:17 AM
paulwalker-arm added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
13650–13652

Up to you but this change is not needed because Op1 and Op2 must be the same type.

This revision is now accepted and ready to land.Fri, Apr 30, 3:17 AM

@paulwalker-arm, Thanks for the review, I added D101593 which removes index_vector, FYI.

This revision was landed with ongoing or failed builds.Fri, Apr 30, 4:14 AM
This revision was automatically updated to reflect the committed changes.