Page MenuHomePhabricator

[SelectionDAG][VP] Add splitting/widening for VP_LOAD and VP_STORE
ClosedPublic

Authored by frasercrmck on Jan 13 2022, 9:23 AM.

Details

Summary

Original patch by @hussainjk.

This patch was split off from D109377 to keep vector legalization
(widening/splitting) separate from vector element legalization
(promoting).

While the original patch added a third overload of
SelectionDAG::getVPStore, this patch takes the liberty of collapsing
those all down to 1, as three overloads seems excessive for a
little-used node.

The original patch also used ModifyToType in places, but that method
still crashes on scalable vector types. Seeing as the other VP
legalization methods only work when all operands need identical
widening, this patch follows in that vein.

Diff Detail

Event Timeline

frasercrmck created this revision.Jan 13 2022, 9:23 AM
frasercrmck requested review of this revision.Jan 13 2022, 9:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2022, 9:23 AM

Is it worth testing a case that needs widening followed by splitting. Basically any non-power of 2 that rounds up to something that needs splitting. Bonus points if it needs enough splitting to trigger the HiIsEmpty is empty case.

I'm not sure we should be widening masked loads/stores or VP loads/stores to power of 2 that needs splitting. It causes the splitting logic to need to deal with odd memory VTs and infer a lot from the memory VT. I think we should be "widening" the load/stores by blocking it into legal pieces as we go like we do for regular loads/stores. But I've thought that for 2 years and haven't done anything about it. So that shouldn't block this patch.

  • add testing of widening+splitting

Is it worth testing a case that needs widening followed by splitting. Basically any non-power of 2 that rounds up to something that needs splitting. Bonus points if it needs enough splitting to trigger the HiIsEmpty is empty case.

It's definitely worth it in that it showed a few things we've yet to solve!

For starters, I couldn't test ret <vscale x 33 x double> which, since it uses all vector return registers, is lowered as a regular store <vscale x 33 x double> and we can't widen that. I did add scalable STORE widening via VP_STORE but that's only if the type is legal. Maybe we can loosen that restriction, I dunno (that kinda goes against what you're recommending later about legalization via widening, though).

Another issue I found was in lowering a <33 x double> vp.store function, where we run out of registers during register allocation. The argument lowering looks terrible but I haven't investigated what's going on.

Anyways, I did manage to add some testing, at least.

This revision is now accepted and ready to land.Jan 14 2022, 12:27 PM
This revision was landed with ongoing or failed builds.Jan 15 2022, 3:51 AM
This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.Jan 15 2022, 5:13 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
4373

545624973628dfca1b7f86b039e0d81afc397eb8 fixed -Wunused-variable in -DLLVM_ENABLE_ASSERTIONS=off builds