This is an archive of the discontinued LLVM Phabricator instance.

[VP] Add splitting for VP_STRIDED_STORE and VP_STRIDED_LOAD
ClosedPublic

Authored by loralb on Mar 16 2022, 3:08 AM.

Details

Summary

Following the comment's thread of D117235, I added checks for the widening + splitting case, which also causes a split with one of the resulting vectors to be empty. Due to the same issues described in that same thread, the fixed-vectors-strided-store.ll test is missing the widening + splitting case, while the same case in the strided-vpload.ll test requires to manually split the loaded vector.

Diff Detail

Event Timeline

loralb created this revision.Mar 16 2022, 3:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2022, 3:08 AM
loralb requested review of this revision.Mar 16 2022, 3:08 AM
craig.topper added inline comments.Mar 21 2022, 11:15 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1891

Does MemVT for a VP_STRIDED_LOAD ever mismatch with the element count of the result. This GetDependentSplitDestVTs is because of how we widen masked loads, but we aren't doing that same widening for strided loads.

1921

Can the HiIsEmpty case happen?

1939

Doesn't the offset need to include the stride which you can't do just like you can't for scalable vectors.

craig.topper added inline comments.Mar 21 2022, 11:20 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1895

We're inside of type legalization, why wouldn't the stride type get legalized after the split? Though you wouldn't be able to use it in the ISD::MUL below if you didn't extend it first.

loralb edited the summary of this revision. (Show Details)Apr 6 2022, 2:10 AM
loralb added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1891

I am using GetDependentSplitDestVTs because I need the HiIsEmpty flag, I do not know if there is a better way to do it though.

1895

I forgot to add the legalization steps for integer operands. I opened a new revision to address this: D123112

1921

I think that in the case that we have a widening followed by a split it may happen that one of the resulting vectors is empty, similar to what happens for VP_LOAD and VP_STORE. But it also may be that I am misunderstanding how the splitting for strided intrinsics should work.

loralb updated this revision to Diff 420751.Apr 6 2022, 2:38 AM

Changelog:

  • Address some of the review comments

A new parent revision was added: now there is no need anymore to manually handle the stride operand expansion/truncation.

craig.topper added inline comments.Apr 6 2022, 8:35 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1891

I don't know what I was thinking before. The memory VT keeps the original type so I guess this is correct.

craig.topper added inline comments.Jul 21 2022, 12:20 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1936

This will create a MachineMemOperand with size based on HiMemVT, but I don't think that's correct. We need UnknownSize.

loralb added inline comments.Aug 1 2022, 4:11 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1936

In getStridedLoadVP the MachineMemOperand is created using MemoryLocation::UnknownSize, isn't this correct?
Anyway, I noticed how in SplitVecOp_VP_STRIDED_STORE I actually create the MachineMemOperand before invoking getStridedStoreVP, so for consistency I could do the same here, what do you think?

craig.topper added inline comments.Aug 12 2022, 12:26 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1936

I didn't realize getStridedLoadVP did that. I think I noticed the inconsistency between store and load and thought there was something wrong. I think we should make them consistent.

loralb updated this revision to Diff 453233.Aug 17 2022, 1:45 AM

Changelog:

  • Create the MachineMemOperand before invoking getStridedLoadVP
  • Rebase
loralb marked an inline comment as done.Aug 17 2022, 1:46 AM
This revision is now accepted and ready to land.Aug 18 2022, 10:56 AM
This revision was landed with ongoing or failed builds.Aug 19 2022, 6:16 PM
This revision was automatically updated to reflect the committed changes.