Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
17905 | Can we still use ConcatVector here and then lower it performConcatVectorsCombine or td file? |
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. 🤔 |
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. |
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. |
- Remove unnecessary assert.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
17919 | I don't believe so, no. |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
17905 | Still we can keep ConcatVector here, and lowering to SPLICE in performConcatVectorsCombine at later phase. |
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. |
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. |
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. |
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? |
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. |
- 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. |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
17905 | make sense to me. Meanwhile, is there any plan to improve such issue? @paulwalker-arm |
- Remove handling of extra operands that don't exist in splice combine
- Allow splice combine to operate on fixed width vectors
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. |
Perhaps paranoia but is it worth adding assert(N->getOpcode() == AArch64ISD::SPLICE && "Unexepected Opcode!");?