This is an archive of the discontinued LLVM Phabricator instance.

[SVE][CodeGen] Legalisation of INSERT_VECTOR_ELT for scalable vectors
ClosedPublic

Authored by kmclaughlin on Jul 29 2020, 10:34 AM.

Details

Summary

When the result type of insertelement needs to be split,
SplitVecRes_INSERT_VECTOR_ELT will try to store the vector to a
stack temporary, store the element at the location of the stack
temporary plus the index, and reload the Lo/Hi parts.

This patch does the following to ensure this works for scalable vectors:

  • Sets the StackID with getStackIDForScalableVectors() in CreateStackTemporary
  • Adds an IsScalable flag to getMemBasePlusOffset() and scales the offset by VScale when this is true
  • Ensures the immediate is clamped correctly by clampDynamicVectorIndex so that we don't try to use an out of range index

Diff Detail

Event Timeline

kmclaughlin created this revision.Jul 29 2020, 10:34 AM
kmclaughlin requested review of this revision.Jul 29 2020, 10:34 AM

There's probably some argument for writing some aarch64-specific code to do a double-wide insert in registers, but maybe not common enough to be worth worrying about.

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1507

getFixedSize()?

1517

Can we pass a TypeSize here, instead of passing the offset and scalable bit separately?

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7207

getFixedSize()?

7212

The non-scalable path uses masking; I guess you can't do that here because the element count might not be a power of two? Given that, the end result here isn't great, but I can't come up with anything better.

7237

getFixedSize()?

david-arm added inline comments.Jul 30 2020, 12:21 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1459–1460

Maybe call getVectorMinNumElements() instead?

kmclaughlin marked 4 inline comments as done.
  • Replaced uses of getKnownMinSize() with getFixedSize() where appropriate
kmclaughlin added inline comments.Jul 30 2020, 9:53 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1517

Since TypeSize uses uint64_t, I thought it would be better to pass in IsScalable as a separate argument so that we can keep the Offset argument as a signed value.

kmclaughlin added inline comments.Jul 30 2020, 10:00 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7212

Yes, masking wasn't used here since we can't be sure that NElts is a power of two

efriedma added inline comments.Jul 30 2020, 12:13 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1517

We could introduce a dedicated TypeOffset type if you think that's really an issue. I suspect getMemBasePlusOffset is rarely, if ever, used with a negative offset, though. Maybe add an overload that takes a TypeSize and go from there.

I really want to avoid getKnownMinSize() where it isn't necessary; it's going to be a lot easier to make mistakes passing around possibly-scaled offsets as bare integers.

  • Rebased and made this patch dependent on D85220, which changes the type of the Offset argument passed to getMemBasePlusOffset() to TypeSize
kmclaughlin added inline comments.Aug 4 2020, 10:14 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1517

I had a look through the uses of getMemBasePlusOffset (and getObjectPtrOffset) and I think you're right that it is not used with negative offsets, so I created a separate patch which changes the type of the Offset argument rather than adding an overload

david-arm added inline comments.Aug 5 2020, 2:42 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1515–1516

It might be cleaner to do this now:

TypeSize IncrementSize = LoVT.getSizeInBits() / 8;

then pass that directly to getMemBasePlusOffset?

1520

I think this might be broken now since the offset doesn't make sense for scalable vectors. Maybe you can replace the DAG.getMemBasePlusOffset call with the new IncrementPointer helper function? That would also give you a MachinePointerInfo object that you can pass to the load here? I've also started making use of IncrementPointer in my patches too.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7246

nit: Stray new line here.

kmclaughlin marked an inline comment as done.
  • Replaced call to getMemBasePlusOffset in LegalizeVectorTypes.cpp with IncrementPointer
kmclaughlin marked 2 inline comments as done.Aug 6 2020, 9:11 AM
kmclaughlin added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1515–1516

I used your suggestion below to replace the call to getMemBasePlusOffset with a call to the new IncrementPointer function, which removed the need to calculate IncrementSize here.

1520

I've replaced the call here as suggested, though the changes I've made in this patch to getMemBasePlusOffset are still necessary in ensuring the immediate is clamped correctly (it is used by getVectorElementPointer)

david-arm accepted this revision.Aug 7 2020, 1:40 AM

LGTM!

llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1521

nit: Maybe drop the Load->getMemOperand()->getFlags() and Load->getAAInfo() arguments, since the original version didn't have them?

This revision is now accepted and ready to land.Aug 7 2020, 1:40 AM
This revision was automatically updated to reflect the committed changes.
kmclaughlin marked an inline comment as done.

Thanks for reviewing this patch, @efriedma & @david-arm!