This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Improve codegen for fixed length vector concat
ClosedPublic

Authored by bsmith on May 14 2021, 7:23 AM.

Diff Detail

Unit TestsFailed

Event Timeline

bsmith created this revision.May 14 2021, 7:23 AM
bsmith requested review of this revision.May 14 2021, 7:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2021, 7:23 AM
Matt added a subscriber: Matt.May 14 2021, 12:12 PM
junparser added inline comments.May 17 2021, 12:20 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17791

Can we still use ConcatVector here and then lower it performConcatVectorsCombine or td file?

joechrisellis added inline comments.May 17 2021, 2:06 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17797

nit: no harm in having SrcOp1VT and SrcOp2VT, I guess -- I know SrcOp2VT would only be used in the assertion but it seems reasonable to have both I'd say. 🙂

17799

Can we add a message to the assertion?

17805

Just a question -- can we have CONCAT_VECTOR with a single operand? Doesn't seem like it. 🤔

paulwalker-arm added inline comments.May 17 2021, 2:18 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17799

I'd go the other way I just remove the assert. By definition all the operands of ISD::CONCAT_VECTORS must have the same type so it's not really the job of this function to validate the DAG is not malformed.

bsmith added inline comments.May 17 2021, 3:44 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17791

This is consistent with how all other fixed length operations are handled. The issue with trying to do this in tablegen, is that we'd have to add patterns for every single <= 2048-bit vector type, which is just going to cause an explosion of patterns.

bsmith updated this revision to Diff 345818.May 17 2021, 3:54 AM
bsmith marked an inline comment as done.
  • Remove unnecessary assert.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17805

I don't believe so, no.

junparser added inline comments.May 17 2021, 7:49 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17791

Still we can keep ConcatVector here, and lowering to SPLICE in performConcatVectorsCombine at later phase.

paulwalker-arm added inline comments.May 18 2021, 3:07 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17791

@junparser This wouldn't follow the design we have for SVE's fixed-length code generation support. Our intent is to exit operation legalisation with all fixed-length operations replaced with scalable vector equivalents.

bsmith added inline comments.May 18 2021, 3:20 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17791

@junparser If what you meant here is that we could lower to a scalable CONCAT_VECTOR and then later lower/match that to a splice instruction, that doesn't work well either as we won't have a sensible way to get at the fixed sizes being used which are required to produce the correct predicate for the instruction, hence making it very difficult to later lower/match to a splice.

paulwalker-arm added inline comments.May 18 2021, 8:12 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17800

I'm wondering if there needs to be this distinction or if we can just have ContainerVT. I say this because the container is based on the element type and since the Src and Dst element types for CONCAT_VECTORS must be the same, then the container type will also be the same.

17805

It is allowed for CONCAT_VECTORS to have a single operand, but this get's folded away during construction and so doesn't need special handling. That said, I do think it's better for the getNumOperands() check to be done earlier before extracting specific operands via Op.getOperand(#num).

17811–17813

This looks like something that can be extracted into a target specific dag combine (i.e. SPLICE pg, op1, undef -> op1) being a common scalable vector transform, and means the lowering code will be simpler. You'll see we do something similar for AArch64ISD::UZP1.

bsmith added inline comments.May 18 2021, 8:45 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17811–17813

I've since noted that I actually need support for more than 2 operands in the undef case (i.e. concat_vectors op, undef, undef, undef). Given that, does it still make sense to move this out into a dag combine for SPLICE?

paulwalker-arm added inline comments.May 18 2021, 8:58 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17811–17813

My gut feeling is that we should lower CONCAT_VECTORS that take more than two operands iteratively (i.e. concat_vectors(op, undef, undef, undef)->concat_vectors(concat_vectors(op, undef), concat_vectors(undef, undef)) and then lower the final two operand CONCAT_VECTORS to SPLICE. Do you see any downside with this? I'll admit to thinking this would have somehow already been the case, so am a little surprised to see them during lower operation.

bsmith updated this revision to Diff 346455.May 19 2021, 7:36 AM
bsmith marked 4 inline comments as done.
  • Move splice undef optimisation to a target specific dag combine
  • Handle >2 operand concat_vectors nodes.
  • Some minor code tidy ups.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
14571–14572

Is this necessary? I guess the node will only every be used by scalable vectors but there's nothing "scalable only" about the transformation.

14574–14576

Not sure what I'm missing here as I expected this to be just if (N->getOperand(2).isUndef()).

17820

Feel free to ignore but the use of Op here seems arbitrary.

junparser added inline comments.May 19 2021, 5:50 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17791

make sense to me. Meanwhile, is there any plan to improve such issue? @paulwalker-arm

bsmith updated this revision to Diff 346682.May 20 2021, 3:24 AM
bsmith marked 2 inline comments as done.
  • Remove handling of extra operands that don't exist in splice combine
  • Allow splice combine to operate on fixed width vectors
paulwalker-arm accepted this revision.May 20 2021, 10:03 AM
paulwalker-arm added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
14568

Perhaps paranoia but is it worth adding assert(N->getOpcode() == AArch64ISD::SPLICE && "Unexepected Opcode!");?

17791

@junparser It's not clear to me what issue needs solving here.

This revision is now accepted and ready to land.May 20 2021, 10:03 AM
junparser added inline comments.May 20 2021, 7:01 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17791

As @bsmith said, Is there a snesible way to keep general ISD node, and transform to AArch64 ISD node at last for SVE's fixed-length code generation?

This revision was landed with ongoing or failed builds.May 24 2021, 4:59 AM
This revision was automatically updated to reflect the committed changes.