Page MenuHomePhabricator

[ValueTypes] Add MVTs for v256i16 and v256f16
ClosedPublic

Authored by frasercrmck on Mon, May 3, 10:35 AM.

Details

Summary

This patch adds the two MVTs to fix a legalizer crash when using vector
shuffles of <256 x i16> and <128 x i16> on RISC-V. The legalizer can't
promote the operand of v256i32 = any_extend_vector_inreg v128i16.

Diff Detail

Event Timeline

frasercrmck created this revision.Mon, May 3, 10:35 AM
frasercrmck requested review of this revision.Mon, May 3, 10:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, May 3, 10:35 AM

This does feel like an imbalance in the legal RISCV vector types. Is it really worth adding another legalization case rather than just adding more simple vector types or making v256i32 illegal?

This does feel like an imbalance in the legal RISCV vector types. Is it really worth adding another legalization case rather than just adding more simple vector types or making v256i32 illegal?

Yeah it's definitely an imbalance. I've been thinking that we need to round out the legal RVV vector types to an agreed-upon max vector length, like 512 or 1024 bytes.

But then again this is a crash in LLVM where there shouldn't be one, so I was kind of assuming that we'd do both. The downside, though, is that we might not have enough test cases to prevent regressions in this legalizer case.

If I had to choose one, I'd add more MVTs up to 512B/1024B depending on what people generally see as a useful maximum. I'm willing to go with the majority.

TBH I think extending the MVT simple types list would be easier.

Okay, sounds like that's the way to go, thanks. It's a shame we don't have a way of testing the various legalizers without a suitable target.

RKSimon requested changes to this revision.Tue, May 4, 6:46 AM
This revision now requires changes to proceed.Tue, May 4, 6:46 AM
  • add v256i16 and v256f16
frasercrmck retitled this revision from [LegalizeIntegerTypes] Promote EXTEND_VECTOR_INREG operands to [ValueTypes] Add MVTs for v256i16 and v256f16.Tue, May 4, 8:12 AM
frasercrmck edited the summary of this revision. (Show Details)
craig.topper added inline comments.Tue, May 4, 8:46 AM
llvm/include/llvm/CodeGen/ValueTypes.td
185

Should we add blank lines between different element widths here too?

frasercrmck marked an inline comment as done.
  • add blank lines between scalable-vector elt tys
llvm/include/llvm/CodeGen/ValueTypes.td
185

Ah yeah I missed that one, thanks!

RKSimon accepted this revision.Tue, May 4, 9:00 AM

LGTM - cheers

This revision is now accepted and ready to land.Tue, May 4, 9:00 AM
frasercrmck edited the summary of this revision. (Show Details)Tue, May 4, 9:27 AM
This revision was landed with ongoing or failed builds.Tue, May 4, 10:13 AM
This revision was automatically updated to reflect the committed changes.