Page MenuHomePhabricator

[SelectionDAG] Widen scalable-vector loads/stores via VP_LOAD/VP_STORE

Authored by frasercrmck on Oct 6 2021, 10:39 AM.



This patch fixes a compiler crash when widening scalable-vector loads
and stores which end up breaking down to element-wise store operations.
It does so by providing a way for targets with support for
vector-predicated loads and stores to use those instead. By widening the
operation but maintaining the original effective operation length via
the EVL, only the intended vector elements are loaded or stored.

This method should in theory be possible and even preferred for
fixed-length vector types, but all fixed-length types can be broken down
into their elements, and regardless I have observed regressions in the
generated code when doing so. I believe this is simply due to
VP_LOAD/VP_STORE not being up to par with LOAD/STORE in terms of
optimization. It does improve performance on smaller self-contained
examples, however, so the potential is there.

While the only target that benefits from this is RISCV, the legalization
is generic and so was placed centrally.

Diff Detail

Event Timeline

frasercrmck created this revision.Oct 6 2021, 10:39 AM
frasercrmck requested review of this revision.Oct 6 2021, 10:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2021, 10:39 AM
craig.topper added inline comments.Oct 7 2021, 11:08 AM

recusion -> recursion?


I'm distrusting of changeVectorElementType. If WideVT happens to be an MVT, the changed type must also be an MVT since there is no way to get the LLVMContext from the MVT to make an EVT for the i1 vector.

Can we use EVT::getVectorVT here instead?

40 ↗(On Diff #377606)

How are we able to widen this load? Looks like we inferred an align of 4. I think we used that alignment to infer that it was ok to widen using the logic we have for fixed vectors. But I don't think that can work with vscale.

  • fix typo
  • use EVT::getVectorVT
frasercrmck marked 2 inline comments as done.Oct 8 2021, 8:43 AM
frasercrmck added inline comments.

Good spot, thanks.


Quite right. It was my intention to fix that up before posting the patch, but I forgot

40 ↗(On Diff #377606)

I'm glad you pointed this out. I was pretty surprised by this (I was expecting it to crash like stores do). I then somehow convinced myself that it was okay but that was probably wishful thinking. Is the assumption that the hardware's addressable memory is always a multiple of the alignment and so if <3 x i8> is aligned to 4 there must be an extra byte that it can load without faulting?

If so I agree that vscale doesn't work like this. Do you think this should be patched quickly to crash like stores currently do? Feels pretty blunt but it's better than silently generating wrong code.

craig.topper added inline comments.Oct 8 2021, 11:21 AM
40 ↗(On Diff #377606)

Yeah if the alignment is >= than the size of the widened load it assumes it won't fault.

An easy fix might be to squash the alignment to 0 for scalable vectors here. I assume that's why volatile crashes.

// Allow wider loads if they are sufficiently aligned to avoid memory faults   
// and if the original load is simple.                                         
unsigned LdAlign = (!LD->isSimple()) ? 0 : LD->getAlignment();
frasercrmck marked 2 inline comments as done.Oct 15 2021, 6:52 AM
frasercrmck added inline comments.
40 ↗(On Diff #377606)

I've (finally) created D111885 to fix the wrong code, and to serve as a basis for this work.

  • rebase on top of prep patch
  • add VP_LOAD support too
frasercrmck retitled this revision from [SelectionDAG] Widen scalable-vector stores via VP_STORE to [SelectionDAG] Widen scalable-vector loads/stores via VP_LOAD/VP_STORE.Oct 18 2021, 8:52 AM
frasercrmck edited the summary of this revision. (Show Details)
  • rebase again
  • rebase
  • fix accidental unreachable due to bad rebasing
craig.topper added inline comments.Tue, Nov 9, 8:25 AM

I thought you fixed vp.load with all ones mask in D113022. What is still missing?

frasercrmck marked an inline comment as done.Tue, Nov 9, 8:42 AM
frasercrmck added inline comments.

Ach I thought I'd rebased. Done now, cheers.

This revision is now accepted and ready to land.Tue, Nov 9, 8:45 AM
frasercrmck marked an inline comment as done.Tue, Nov 9, 8:46 AM