This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Make WidenVecRes_EXTRACT_SUBVECTOR work for scalable vectors.
ClosedPublic

Authored by sdesmalen on Sep 22 2021, 7:21 AM.

Details

Summary

The legalizer handles this by breaking up an EXTRACT_SUBVECTOR into
smaller parts, and combines those together, padding the result with
UNDEF vectors, e.g.

nxv6i64 extract_subvector(nxv12i64, 6)
<->
nxv8i64 concat(
  nxv2i64 extract_subvector(nxv16i64, 6)
  nxv2i64 extract_subvector(nxv16i64, 8)
  nxv2i64 extract_subvector(nxv16i64, 10)
  nxv2i64 undef)

Diff Detail

Event Timeline

sdesmalen created this revision.Sep 22 2021, 7:21 AM
sdesmalen requested review of this revision.Sep 22 2021, 7:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2021, 7:21 AM
david-arm added inline comments.Sep 23 2021, 4:07 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
4108

Hi @sdesmalen, I think we're assuming here that IdxVal is always going to be a multiple of GCD, which may be true. However, I wonder if it's worth adding an assert for this to ensure we're always following the lang ref for EXTRACT_SUBVECTOR.

sdesmalen updated this revision to Diff 374569.Sep 23 2021, 8:33 AM

Added assert for Idx.

Rebased patch onto D110523 (which means the tests have changed)

david-arm accepted this revision.Sep 27 2021, 1:36 AM

LGTM! The extract_nxv6f16_nxv12f16_6 test looks right now, thanks @sdesmalen! I haven't gone through the predicate tests because it's a nightmare, but since the fp16 tests look right I assume the algorithm is correct for predicates too.

This revision is now accepted and ready to land.Sep 27 2021, 1:36 AM

I'd be interested in seeing some RISCV tests to improve the coverage (but I won't block on it)

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

Nit: is this message too general now? It implies we can't do anything, ever.

I'd be interested in seeing some RISCV tests to improve the coverage (but I won't block on it)

I'm happy to add a test, just wasn't entirely sure what types are legal for RVV. Would the same extract <vscale x 6 x half> from <vscale x 12 x half> be suitable?

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

Ah that wasn't my intention, I meant it more as "the code as it is today doesn't know how to widen the result of extract_subvector for scalable vectors". I think the message is correct though, since LLVM doesn't know how to widen it and it avoids falling into the fixed-width code below.

I'd be interested in seeing some RISCV tests to improve the coverage (but I won't block on it)

I'm happy to add a test, just wasn't entirely sure what types are legal for RVV. Would the same extract <vscale x 6 x half> from <vscale x 12 x half> be suitable?

Thanks, yeah that'd need widening. Broadly speaking all power-of-two element counts 1+ are either legal or will need splitting. Anything else will be widened. We don't have bfloat though.

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

Hmm, from my perspective it's just that this method is WidenVecRes_EXTRACT_SUBVECTOR and this patch is enabling it to widen the result of extract_subvector for scalable vectors -- at least for those where the part doesn't also need widening. I agree there needs to be a fatal error here, just I thought that a user seeing this error may think something else is going on. I dunno, I may have missed the mark here.

Added a similar test for RVV.

frasercrmck accepted this revision.Sep 29 2021, 2:02 AM

LGTM, thanks for adding some RVV tests, @sdesmalen.

llvm/test/CodeGen/RISCV/rvv/extract-subvector.ll
486

I'm not sure this vslideup.vi is completely necessary but I think it's correct.

This revision was landed with ongoing or failed builds.Sep 29 2021, 3:56 AM
This revision was automatically updated to reflect the committed changes.

Cheers for the review @frasercrmck and @david-arm!