Page MenuHomePhabricator

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

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

Diff Detail

Event Timeline

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

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

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

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. 🙂

17913

Can we add a message to the assertion?

17919

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

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

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.Mon, May 17, 3:44 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17905

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.Mon, May 17, 3:54 AM
bsmith marked an inline comment as done.
  • Remove unnecessary assert.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17919

I don't believe so, no.

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

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

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

@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.Tue, May 18, 3:20 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17905

@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.Tue, May 18, 8:12 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17914

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.

17919

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).

17925–17927

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.Tue, May 18, 8:45 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17925–17927

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.Tue, May 18, 8:58 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17925–17927

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.Wed, May 19, 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
14583–14584

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

14586–14588

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

17934

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

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

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

bsmith updated this revision to Diff 346682.Thu, May 20, 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.Thu, May 20, 10:03 AM
paulwalker-arm added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
14580

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

17905

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

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

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.Mon, May 24, 4:59 AM
This revision was automatically updated to reflect the committed changes.