This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add support for VECTOR_REVERSE for scalable vector types.
ClosedPublic

Authored by craig.topper on Feb 26 2021, 11:38 PM.

Details

Summary

I've left mask registers to a future patch as we'll need
to convert them to full vectors, shuffle, and then truncate.

Diff Detail

Event Timeline

craig.topper created this revision.Feb 26 2021, 11:38 PM
craig.topper requested review of this revision.Feb 26 2021, 11:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2021, 11:38 PM
Herald added a subscriber: MaskRay. · View Herald Transcript
craig.topper added inline comments.Feb 26 2021, 11:44 PM
llvm/utils/TableGen/CodeGenDAGPatterns.cpp
743

This hack is to prevent tablegen from throwing a bunch of warnings about assuming a vector type is not scalable. This occurs because of my use of SDTCisSameSizeAs in the type profile for vrgather_vv_vl in tablegen.

SmallSet defaults to std::less as its comparison function. But TypeSize doesn't implement operator< natively. But it appears to match to the operator< for uint64_t since there's a conversion operator to uint64_t in TypeSize. But that only works for fixed vectors.

To workaround this, I explicitly split up the TypeSize into a std::pair. Adding an operator< to TypeSize didn't seem like a good idea. Another option might be to switch to SmallDenseSet and add a DenseMapInfo for TypeSize.

frasercrmck added inline comments.Mar 4 2021, 1:56 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1519

I suppose we'll have to promote to i16 for this case?

llvm/utils/TableGen/CodeGenDAGPatterns.cpp
743

Hmm. I agree that operator< for TypeSize doesn't sound right. I can see the benefit of having a DenseMapInfo for TypeSize. I guess another option is to overload the comparison for this one case? That's quite short-sighted though.

craig.topper added inline comments.Mar 4 2021, 9:18 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1519

Yeah, but we don't really have a good way of know if it is needed without just always doing it or deciding based on whether -riscv-vector-bits-max is set and what its value is?

We also can't promote to i16 without splitting for lmul=8.

Use a custom comparator struct in tablegen. Thought about a DenseMap, but wasn't sure what to use for empty key and tombstone key.

khchen added a comment.Mar 5 2021, 7:11 AM

Overall LGTM, waiting for @frasercrmck's comment.

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

Agree, it's really a problem, unfortunately vrgatherei16 can not support i8m8 case directly.

I think maybe compiler could emit a warning for non-workable case if user specific -riscv-vector-bits-max.

frasercrmck added inline comments.Mar 8 2021, 2:01 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1519

Yeah, nothing really works well in this situation. It's just that with <vscale x 64 x i8> being legal it's not a problem for the future - it would be silently generating wrong code right now on many architectures.

What if, for now, we decided not to support i8 unless -riscv-vector-bits-max was passed?

llvm/utils/TableGen/CodeGenDAGPatterns.cpp
718

I think this is an improvement over the std::pair.

Handle VLMAX>=256 for SEW=8 by checking -riscv-v-vector-bits-max.

frasercrmck added inline comments.Mar 9 2021, 3:45 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2827

Is there an issue with using CONCAT_VECTORS?

craig.topper added inline comments.Mar 9 2021, 6:44 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2827

We don’t seem to have implemented CONCAT_VECTORS for scalable vectors yet so it went to the stack.

frasercrmck accepted this revision.Mar 9 2021, 6:55 AM

LGTM

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

Ah of course; we only added it for fixed-length vectors since it's common during legalization.

This revision is now accepted and ready to land.Mar 9 2021, 6:55 AM
This revision was landed with ongoing or failed builds.Mar 9 2021, 10:03 AM
This revision was automatically updated to reflect the committed changes.