Page MenuHomePhabricator

[AArch64][SVE] Add SPLAT_VECTOR ISD Node
ClosedPublic

Authored by huntergr on Jun 5 2018, 5:40 AM.

Details

Summary

Adds a new ISD node to replicate a scalar value across all elements of a vector. This is needed for scalable vectors, since BUILD_VECTOR cannot be used.

At present I only use this node for scalable vectors; although the default expand works and generates a build vector with the appropriate value, some backends expect a different canonical form in SDag for a splat. I suspect the code to generate those shuffles will need to be replicated in the expansion (or possibly moved to a shared function) before this can be used by default for all backends.

I've only added legalization for i8/i16/i32/i64 types in AArch64 for now.

Diff Detail

Event Timeline

huntergr created this revision.Jun 5 2018, 5:40 AM
This comment was removed by rkruppe.
huntergr planned changes to this revision.Mar 22 2019, 3:02 AM

Needs to be updated with better isel patterns (and possibly a new ISD?), instead of just the bare minimum required for demonstration.

huntergr updated this revision to Diff 222386.Sep 30 2019, 4:04 AM
huntergr retitled this revision from [AArch64][SVE] Add SplatVector Intrinsic to [AArch64][SVE] Add SPLAT_VECTOR ISD Node.
huntergr edited the summary of this revision. (Show Details)
huntergr added reviewers: greened, cameron.mcinally.
huntergr set the repository for this revision to rG LLVM Github Monorepo.
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2019, 4:04 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
jmolloy requested changes to this revision.Sep 30 2019, 4:41 AM
jmolloy added a subscriber: jmolloy.

Hi Graham,

I really like the addition of a new ISD opcode for this. I do have one inline comment.

James

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
3548

It feels to me like this is a canonicalization that should be a DAGCombine rather than in the builder. Is this here specifically because the ISD::vector_shuffle is not representable with scalable vectors?

If not, I'd prefer this be moved to a canonical dag combine. In general, I feel logic here should be at a minimum because it runs before targets can override or pattern match themselves.

This revision now requires changes to proceed.Sep 30 2019, 4:41 AM
huntergr marked an inline comment as done.Sep 30 2019, 5:28 AM
huntergr added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
3548

It's because BUILD_VECTOR cannot be used for scalable vectors, and if we tried to use VECTOR_SHUFFLE, we'd still need to generate a mask with an unknown number of elements which we can't do at the moment.

I've limited it to scalable vectors only for now precisely because this does break some unit tests if I enable it for all targets -- they do get a splat, but not the expected splat. It may be better to move this code to SelectionDAG::getVectorShuffle near the code that checks TLI->hasVectorBlend(), which also seems to match and transform splats early on.

We do have a VECTOR_SHUFFLE_VAR node downstream which takes a third argument as a mask, and with nodes for vscale and stepvector we're able to generate the masks we need -- but that's for a separate discussion and patch.

huntergr marked an inline comment as not done.Sep 30 2019, 5:28 AM
jmolloy added inline comments.Sep 30 2019, 5:58 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
3548

I suppose an extension of my comment is that this introduces a super-cool new node that a bunch of backends have wanted for a while, but doesn't plumb in any canonicalization to it for targets that aren't scalable vectors.

huntergr added inline comments.Oct 2 2019, 2:39 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
3548

Is there a specific change you'd like to see? I'm not quite sure what's needed at this point.

jmolloy added inline comments.Oct 2 2019, 2:43 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
3548

The specific change I'd like to see is canonicalization of BUILD_VECTOR<x,x,x,x,x...> into SPLAT_VECTOR in DAGCombine. I'm opposed to taking a canonical pattern (BUILD_VECTOR<x,x,x,...>) and adding a new node to represent it (SPLAT_VECTOR) without canonicalizing the old pattern to the new node.

I think this will cause duplication of patterns in DAGCombine where BUILD_VECTOR<> has a set of optimizations and SPLAT_VECTOR has a different set, for no reason.

Hi James,

So I implemented a combine to do this, but ran into a problem -- namely that the design suggested by @hfinkel in the llvm-dev discussion (to expand to build_vector when the backend hasn't marked the splat_vector operation as supported) conflicts with the combine and ends up in an infinite loop.

For now I've guarded the combine with a check on whether the operation action has been changed from Expand for that type. Would this be acceptable?

huntergr updated this revision to Diff 223004.Oct 3 2019, 6:55 AM
  • Added DAG combine to canonicalize a build_vector splat into a splat_vector if the target supports the splat_vector operation (by overriding the default Expand legalization action)

This needs more tests - other targets, some legalization tests

RKSimon added inline comments.
llvm/include/llvm/CodeGen/ISDOpcodes.h
410

Please can you add info on implicit truncation (same as we do for SCALAR_TO_VECTOR/BUILD_VECTOR/INSERT_VECTOR_ELT)

jmolloy accepted this revision.Oct 17 2019, 1:27 AM

Thanks Graham for taking my feedback on board. I agree with Simon's request too.

I'm not convinced I agree with Roman's requests for tests on more targets; this feature is opt-in and the canonicalization is only enabled when a target marks it non-expand, which no target has apart from AArch64.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
17632

nit: Extraneous parens around the cast<>.

17633

This is probably worth a comment. I suppose we don't convert if the splatted value is undef because the entire build_vector is undef and you haven't written all the canonicalizations for that?

llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
1405

I know you've copied the formatting from above, but this formatting is weird and should be clang-formatted.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
3558

Can we have a comment that the DAGCombiner will perform build_vector->splat_vector conversion for non-scalable types?

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

This if-nest is getting a bit deep, could these two ifs be merged?

This revision is now accepted and ready to land.Oct 17 2019, 1:27 AM
RKSimon added inline comments.Oct 17 2019, 1:31 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
17633

This if() might be better as an assert - we have the ISD::allOperandsUndef(N) test earlier in visitBUILD_VECTOR.

huntergr updated this revision to Diff 225414.Oct 17 2019, 6:17 AM
  • Fixed legalization action code, which was trying to use widening for all integer scalable vector types after the new MVT ranges were added
  • Added unit tests for legalizing the result via promotion
  • Minor cleanups, additional comments as requested
huntergr marked 6 inline comments as done.Oct 17 2019, 6:19 AM
This revision was automatically updated to reflect the committed changes.