This is an archive of the discontinued LLVM Phabricator instance.

[VP] Strided loads/stores
ClosedPublic

Authored by loralb on Dec 1 2021, 8:45 AM.

Details

Summary

This patch introduces two new experimental IR intrinsics and SDAG nodes to represent vector strided loads and stores.

Diff Detail

Event Timeline

loralb created this revision.Dec 1 2021, 8:45 AM
loralb requested review of this revision.Dec 1 2021, 8:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2021, 8:45 AM
craig.topper added inline comments.Dec 1 2021, 2:33 PM
llvm/include/llvm/IR/VPIntrinsics.def
256

Is this an existing mistake in the documentation? Can you just push this as an NFC commit? No need to bury it in this patch.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
8117

Since this accesses multiple disjoint memory locations, the Size is not MemVT.getStoreSize(). It probably needs to always be unknown. I guess it would be if the stride were statically known to be 0, but not sure it is worth special casing that.

8250

Same as load, the Size can't be based on the store type.

8306

Same

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

This Size doesn't make sense. I think it needs to be unknown.

7498

Same

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h
574

Can these be SmallVectorImpl<SDValue>?

rogfer01 added inline comments.Dec 1 2021, 10:43 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7465

Does anyone know if we should be passing AAInfo here?

(Non-stride) VPLoad doesn't do that, so we can fix both in a later change if necessary.

loralb updated this revision to Diff 391348.Dec 3 2021, 12:52 AM

ChangeLog:

  • add documentation for the new intrinsics
  • use MemoryLocation::UnknownSize
  • use SmallVectorImpl<SDValue> in functions signatures
loralb marked 7 inline comments as done.Dec 3 2021, 12:57 AM
loralb added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h
574

I'll fix visitVPLoadGather and visitVPStoreScatter above in a later change.

loralb updated this revision to Diff 394256.Dec 14 2021, 8:40 AM
loralb marked an inline comment as done.

Changelog:

  • ensure default alignment behaviour reflects changes in D114423
  • align MemoryLocation retrieval for VPStridedLoad to VPLoad (see D115036)
loralb marked an inline comment as done.Dec 14 2021, 8:42 AM
simoll added inline comments.Dec 15 2021, 3:04 AM
llvm/lib/IR/IntrinsicInst.cpp
511

This will require an explicit return type because of opaque pointers, as in 6213f1dd03e27fd2483f8ac775a346de8e873573

loralb updated this revision to Diff 394547.Dec 15 2021, 6:36 AM

Changelog:

  • use explicit return type in Intrinsic::getDeclaration() for experimental_vp_strided_load
  • get first argument type from first operand in Intrinsic::getDeclaration() for experimental_vp_strided_store
loralb marked an inline comment as done.Dec 15 2021, 6:37 AM
simoll added a project: Restricted Project.Dec 23 2021, 12:40 AM
frasercrmck added inline comments.Jan 27 2022, 8:24 AM
llvm/docs/LangRef.rst
19938

I think mentioning that the align parameter can be used (and correctly handling that further down in SelectionDAG) would be good.

loralb updated this revision to Diff 409936.Feb 18 2022, 9:16 AM

Changelog:

  • Mention that the align parameter attribute can be provided for the base pointer operands
loralb marked an inline comment as done.Feb 18 2022, 9:17 AM
loralb added inline comments.Feb 25 2022, 6:42 AM
llvm/docs/LangRef.rst
19926

Reflecting on how the strided versions work, I was thinking if, semantically speaking, it would be better to define the pointer operand as a pointer to the scalar element type of the data/return operand instead of a pointer to a vector (this is only relevant while opaque pointers are not the default)

simoll added inline comments.Feb 28 2022, 5:23 AM
llvm/docs/LangRef.rst
19926

Agreed. Typed pointers will go away but we shouldn't be sloppy here.

loralb updated this revision to Diff 412014.Mar 1 2022, 1:52 AM

Changelog:

  • Changed the pointer operand type to match the scalar elements type of the data/return operand
loralb marked an inline comment as done.Mar 1 2022, 1:53 AM
simoll added inline comments.Mar 2 2022, 5:39 AM
llvm/docs/LangRef.rst
19954

The stride can have a different number of bits than the ptr.
To make the definition unambiguous, we should mention that the stride is always interpreted as a signed integer and all arithmetic occurs in the pointer type.
This implies that an over-sized stride is truncated and a less-bits-than-the-address stride is always sign extended to the address type.

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 5:39 AM
loralb updated this revision to Diff 413448.Mar 7 2022, 6:50 AM

Changelog:

  • Update documentation
  • Align with recent changes to vp_load and vp_store
loralb marked an inline comment as done.Mar 7 2022, 6:53 AM
simoll accepted this revision.Mar 7 2022, 7:40 AM

LGTM

This revision is now accepted and ready to land.Mar 7 2022, 7:40 AM

I do not have commit access, could anyone commit this for me please?

This revision was landed with ongoing or failed builds.Mar 10 2022, 9:48 AM
Closed by commit rG28cfa764c2e3: [VP] Strided loads/stores (authored by loralb, committed by simoll). · Explain Why
This revision was automatically updated to reflect the committed changes.