Page MenuHomePhabricator

[SelectionDAG] Add llvm.vector.{extract,insert} intrinsics
ClosedPublic

Authored by joechrisellis on Nov 12 2020, 8:49 AM.

Details

Summary

This commit adds two new intrinsics.

  • llvm.vector.insert: used to insert a vector into another vector starting at a given index.
  • llvm.vector.extract: used to extract a subvector from a larger vector starting from a given index.

The codegen work for these intrinsics has already been completed; this
commit is simply exposing the existing ISD nodes to LLVM IR.

Diff Detail

Event Timeline

joechrisellis created this revision.Nov 12 2020, 8:49 AM
joechrisellis requested review of this revision.Nov 12 2020, 8:49 AM
sdesmalen added a subscriber: sdesmalen.

Was there an RFC?
Why should there be two ways to do the same thing?

Presumably you're planning to add some tests?

llvm/docs/LangRef.rst
16147

The meaning of idx is more nuanced that this as shown by the description attached to the ISD node it is equivalent to. I think this is worth calling out in the langref.

llvm/include/llvm/IR/Intrinsics.td
1620

For both intrinsics you'll need to add ImmArg properties for the index parameter. This is a requirement of the ISD node plus we don't want to open up the possibility of variable insertions.

I would expect the intrinsic to canonicalize to shufflevector with the appropriate shuffle mask for the case where both the vectors are fixed-width?

llvm/docs/LangRef.rst
16108

Can you add/change the example for inserting a fixed-width vector into a scalable vector. That is one of the main reasons for adding this intrinsic.

@lebedev.ri: shufflevector only has minimal support for scalable vectors with only the splat case covered (and even that has its quirks). With the recent change to force the mask to be an ArrayRef there is no way to represent arbitrary shuffles and at the same time the implementation forced a requirement that scalable vector data inputs imply a scalable vector result.

@lebedev.ri: shufflevector only has minimal support for scalable vectors with only the splat case covered (and even that has its quirks). With the recent change to force the mask to be an ArrayRef there is no way to represent arbitrary shuffles and at the same time the implementation forced a requirement that scalable vector data inputs imply a scalable vector result.

I still want to see all this explanation be put both into an RFC and into patch's description.

llvm/docs/LangRef.rst
16108

This caught me off guard. What are the semantics of:

declare <4 x float> @llvm.vector.insert.v4f32(<4 x float> %subvec, <4 x float> %vec, i64 %idx)

Is that a full vector copy? I would have expected the subvector VL to be strictly less than the destination VL. E.g.:

declare <4 x float> @llvm.vector.insert.v2f32.v4f32(<2 x float> %subvec, <4 x float> %vec, i64 %idx)
fhahn added a subscriber: fhahn.Nov 12 2020, 10:18 AM

Thanks @lebedev.ri, I incorrectly assumed exposing existing functionality wouldn't require an RFC. I'm putting together an RFC to cover support for other scalable vector shuffles so I'll include these within that.

joechrisellis marked 3 inline comments as done.

Address @sdesmalen and @paulwalker-arm review comments.

  • Add tests.
  • Use ImmArg for idx parameter.
  • Include example of inserting a fixed-width vector into a scalable vector in LangRef.rst.
  • Use more precise description of idx in LangRef.rst.
llvm/docs/LangRef.rst
16103

Other intrinsics using this phrase also indicate in a following sentence what types are supported, I think we should do that here.

16113

The term "subvector" does not appear to be defined anywhere in the langref that I can see. I wonder if the term could be introduced along with scalable vectors in the "Vector Type" section. Alternatively there may be a way to write this sentence without using the jargon "subvector".

For example: "The 'llvm.vector.insert.*' intrinsics insert a non-scalable vector into a scalable vector at the given index. Conceptually, this can be used to build a scalable vector out of non-scalable vectors.".

16140

Same comment here for a sentence describing valid types.

16150

Same comment here for the subvector language.

Suggested language:

"The 'llvm.vector.extract.*' intrinsics extract a non-scalable vector from a scalable vector at the given index. Conceptually, this can be used to decompose a scalable vector into non-scalable parts."

simoll added a subscriber: simoll.Nov 16 2020, 2:37 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
6938–6939

These look reversed to me. The operand order should match that used by the ISD node.

6952–6955

The result type for both extract and insert should be based on the intrinsic's return type. Something like EVT VT = TLI.getValueType(DAG.getDataLayout(), I.getType())

Address @paulwalker-arm's comments.

  • Reverse operand order of insert intrinsic.
  • Base result type on the return types of the intrinsics.

Auto-generate tests.

Fix failing tests (forgot to write stderr to %t).

joechrisellis planned changes to this revision.Nov 24 2020, 4:03 AM

Planning to add canonicalization to shufflevector for fixed-width vectors.

@lebedev.ri, hi! We submitted an RFC for named shuffle intrinsics to the llvm-dev mailing list (here). Do you think this is sufficient? Note there are still a few changes left to do on this patch (mainly moving the intrinsics to the experimental namespace and adding canonicalisation to shufflevector for the fixed case). 😄

  • Implement canonicalisation for the fixed case.
  • Move intrinsic into llvm.experimental namespace.
  • Update documentation and tests accordingly.

Do we need to protect against mismatched element types? Or does legalization handle those exts/truncs?

%retval = call <vscale x 8 x i16> @llvm.experimental.vector.insert.nxv8i16(<vscale x 8 x i16> %vec, <8 x i8> %subvec, i64 0)

Address @cameron.mcinally's comment regarding protecting against mismatched element types.

Do we need to protect against mismatched element types? Or does legalization handle those exts/truncs?

%retval = call <vscale x 8 x i16> @llvm.experimental.vector.insert.nxv8i16(<vscale x 8 x i16> %vec, <8 x i8> %subvec, i64 0)

Good idea -- added some verifier checks for this.

cameron.mcinally accepted this revision.Dec 4 2020, 9:54 AM
cameron.mcinally added a subscriber: ctetreau.

LGTM

I think @ctetreau's "first class citizen" argument on the RFC has merit though. But this patch is a good first step if we're not ready to extend ShuffleVector yet. I personally would like to see ShuffleVector extended eventually, since it would be easier to optimize.

@ctetreau's RFC comment can be found here:

http://lists.llvm.org/pipermail/llvm-dev/2020-December/146981.html

This revision is now accepted and ready to land.Dec 4 2020, 9:54 AM

Rebase atop main.

Fixup tests.

This revision was landed with ongoing or failed builds.Dec 9 2020, 3:09 AM
This revision was automatically updated to reflect the committed changes.