This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Lower experimental_get_vector_length intrinsic to vsetvli for some cases.
ClosedPublic

Authored by craig.topper on May 17 2023, 3:42 PM.

Details

Summary

This patch lowers to vsetvli when the AVL is i32 or XLenVT and
the VF is a power of 2 in the range [1, 64]. VLEN=32 is not supported
as we don't have a valid type mapping for that. VF=1 is not supported
with Zve32* only.

The element width is used to set the SEW for the vsetvli if possible.
Otherwise we use SEW=8.

Diff Detail

Event Timeline

craig.topper created this revision.May 17 2023, 3:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2023, 3:42 PM
craig.topper requested review of this revision.May 17 2023, 3:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2023, 3:42 PM
asb added a comment.May 19 2023, 7:15 AM

I think you meant to mark this as dependent on D149916.

Rebase on parent patch

reames added inline comments.May 25 2023, 12:31 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1185

For fixed vectors, we should be able to statically compute this when the ElementWidth * VF is less than VLEN right? It's fine to do that in a different patch, just want to make sure I'm not missing something.

1188

Why not i16? I don't see anything in the implementation which couldn't be handled via an extend?

6664

The "if possible" bit is bugging me here. Isn't this a hard requirement? VLMAX should be a function of SEW and LMUL. Given the result here depends on VLMAX, don't we have a hard requirement on having a recognized SEW/LMUL combination?

6673

encodeSEW

craig.topper added inline comments.May 25 2023, 12:53 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1185

The ElementWidth doesn't have any real meaning. The vectorizer already picked a VF. The get_vector_length is asking, given that VF, how many elements fit in that type. The width of the elements or VLEN doesn't change that answer.

I guess we could return a lower number but that means we'd never use the entire type the vectorizer is emitting. That seems like we a cost model issue if we actually want to use less than the type the vectorizer picks.

1188

No strong reason. Might need to add setOperationAction(ISD::INTRINSIC_WO_CHAIN, MVT::i16, Custom) to get the legalization to be called.

reames added inline comments.May 25 2023, 1:21 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1185

I see your point for fixed length vectors. Essentially, we can let the vectorizer pick one via cost modeling, and then split as needed. So here, we can report the number of elements even if splitting might be required.

Rebase after removing element width from the intrinsic.

reames accepted this revision.Jun 5 2023, 9:13 AM

LGTM w/comment addressed.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
6660

This depends very heavily on the fact that VLMAX for 1 x i8 and VLMAX for 1 x i64 are the same. It took me a while to convince myself that this was actually true. The former maps to a fractional lmul, the later maps to m1. And as a result, the element width is irrelevant.

Can you add a comment which explains this in a bit more detail? If this is already well explained elsewhere, you can simply point to that comment.

This revision is now accepted and ready to land.Jun 5 2023, 9:13 AM
craig.topper added inline comments.Jun 5 2023, 9:49 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
6660

A lot of this needs to be rewritten using RVVBitsPerBlock. I'll update.

Use RVVBitsPerBlock to define the math.
Add explanatory comments.

craig.topper requested review of this revision.Jun 5 2023, 10:47 AM

Requesting review to make sure the new comments are sufficient.

reames accepted this revision.Jun 5 2023, 10:50 AM

Ok, yeah, that's a lot easier to follow. Thanks!

LGTM

This revision is now accepted and ready to land.Jun 5 2023, 10:50 AM
This revision was landed with ongoing or failed builds.Jun 5 2023, 3:02 PM
This revision was automatically updated to reflect the committed changes.