Page MenuHomePhabricator

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

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



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

Unit TestsFailed

40 msx64 debian > Flang.Semantics::resolve102.f90
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/flang/test/Semantics/ /mnt/disks/ssd0/agent/llvm-project/flang/test/Semantics/resolve102.f90 /mnt/disks/ssd0/agent/llvm-project/build/tools/flang/test/Semantics/Output/resolve102.f90.tmp /mnt/disks/ssd0/agent/llvm-project/build/bin/f18 -intrinsic-module-directory /mnt/disks/ssd0/agent/llvm-project/build/tools/flang/include/flang
60 msx64 debian > LLVM.CodeGen/RISCV/rvv::named-vector-shuffle-reverse.ll
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -mtriple=riscv32 -mattr=+m,+experimental-v,+f,+d,+experimental-zfh -verify-machineinstrs < /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/RISCV/rvv/named-vector-shuffle-reverse.ll | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck --check-prefixes=CHECK
100 msx64 windows > LLVM.CodeGen/RISCV/rvv::named-vector-shuffle-reverse.ll
Script: -- : 'RUN: at line 2'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llc.exe -mtriple=riscv32 -mattr=+m,+experimental-v,+f,+d,+experimental-zfh -verify-machineinstrs < C:\ws\w64\llvm-project\premerge-checks\llvm\test\CodeGen\RISCV\rvv\named-vector-shuffle-reverse.ll | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe --check-prefixes=CHECK

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

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

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


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

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.


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

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?


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

Is there an issue with using CONCAT_VECTORS?

craig.topper added inline comments.Mar 9 2021, 6:44 AM

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



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.