This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Fix crash during during i1 vector bitreverse lowering
ClosedPublic

Authored by luke on Aug 30 2023, 10:12 AM.

Details

Summary

A shuffle of v256i1 with a large enough minimum vlen might make it through type
legalization and into lowering. In this case, zvl1024b was enough. The
bitreverse shuffle lowering would then try to convert this to a v1i256 type
which is invalid (v1i128 exists though, which is why the existing v128i1 tests
were fine).

This patch checks to make sure that the new type is not only legal but also
valid.

Diff Detail

Event Timeline

luke created this revision.Aug 30 2023, 10:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2023, 10:12 AM
luke requested review of this revision.Aug 30 2023, 10:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2023, 10:12 AM

I was thinking about adding v1i256, v1i512, etc. Is it feasible?

craig.topper added inline comments.Aug 30 2023, 10:21 PM
llvm/test/CodeGen/RISCV/rvv/fixed-vectors-shuffle-reverse.ll
5 ↗(On Diff #554768)

This is the wrong command line for the bug. Don't you need +zvl256b? The bug is due to the min vlen, not the max vlen.

craig.topper added inline comments.Aug 30 2023, 10:35 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4229–4230

Can we use EVT ViaVT = EVT::getVectorVT(*DAG.getContext(), EVT::getIntegerVT(*DAG.getContext(), ViaEltSize), 1); so we're not trying to create non-existent MVTs?

luke updated this revision to Diff 554960.Aug 31 2023, 3:36 AM

Use EVTs (should future proof against larger types if they're ever added
too)
Fix test, move it into separate file since changing zvl causes a large
test diff, as all the LMULs change

luke added a comment.Aug 31 2023, 3:37 AM

I was thinking about adding v1i256, v1i512, etc. Is it feasible?

I've updated the diff to use EVTs as per Craig's suggestion, it should be ok if any larger types are added now.

llvm/test/CodeGen/RISCV/rvv/fixed-vectors-shuffle-reverse.ll
5 ↗(On Diff #554768)

Yeah, not sure what I was thinking.

luke marked 2 inline comments as done.Aug 31 2023, 3:38 AM
luke updated this revision to Diff 554963.Aug 31 2023, 3:44 AM

Add a comment to the test case

reames accepted this revision.Aug 31 2023, 10:02 AM

LGTM

This revision is now accepted and ready to land.Aug 31 2023, 10:02 AM
craig.topper accepted this revision.Aug 31 2023, 10:04 AM

LGTM

llvm/test/CodeGen/RISCV/rvv/fixed-vectors-shuffle-reverse-bitrotate.ll
5

corresponding*

This revision was landed with ongoing or failed builds.Aug 31 2023, 11:39 AM
This revision was automatically updated to reflect the committed changes.