This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Improve extract_vector_elt for fixed mask registers.
ClosedPublic

Authored by jacquesguan on Jan 15 2022, 1:19 AM.

Details

Summary

Now the backend promotes mask vector to an i8 vector and extract element from that. We could bitcast to a widen element vector, and extract from it to GPR, then use I instruction to extract the certain bit.

Diff Detail

Event Timeline

jacquesguan created this revision.Jan 15 2022, 1:19 AM
jacquesguan requested review of this revision.Jan 15 2022, 1:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2022, 1:19 AM
craig.topper added inline comments.Jan 15 2022, 11:41 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4296

Use unsigned instead of auto

4298

Ele->Elt would be a more common shortening.

4302

getSizeBits(). XLenVT is always scalar.

4309

getSizeBits(). XLenVT is always scalar.

4312

getSizeBits(). XLenVT is always scalar.

4314

This shift pair can be an AND right?

4329

I don't think you need to pass false here.

4331

Use SDValue instead of auto.

craig.topper added inline comments.Jan 15 2022, 11:46 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4308

This won't work with Zve32 on RV64. A vector XLen elements wouldn't be legal.

craig.topper added inline comments.Jan 15 2022, 11:48 AM
llvm/test/CodeGen/RISCV/rvv/fixed-vectors-extract-i1.ll
68

I don't think I understand what this subtract is doing.

Address comment.

jacquesguan added inline comments.Jan 17 2022, 12:46 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4296

Done.

4298

Done.

4302

Done

4308

I add MaxEEW to get the right largest vector element width that we could have on current target, but we do not support Zve extension now, so I just set it to 64 and add a TODO to remind changing it after having Zve.

4309

Done

4312

Done

4314

Done

4329

Done.

4331

Done.

llvm/test/CodeGen/RISCV/rvv/fixed-vectors-extract-i1.ll
68

We will use vmv.x.s to extract the elment to GPR, and if eew is less than XLEN, the value will be signed extend. For example, for 8 x i1 mask vector, we will have:

GPR bit    | XLEN -1 |--------------| 6 | 5 | 4 | 3 | 2 | 1 | 0 |
mask index |    0    |--------------| 1 | 2 | 3 | 4 | 5 | 6 | 7 |

So if we want to extract the 0th element, we should set extract bit index to XLEN - 1, and otherwise the index = element width - 1 - mask index.

craig.topper added inline comments.Jan 17 2022, 1:21 AM
llvm/test/CodeGen/RISCV/rvv/fixed-vectors-extract-i1.ll
68

Why would the mask bits be in reversed order?

Address comment.

jacquesguan added inline comments.Jan 17 2022, 4:29 AM
llvm/test/CodeGen/RISCV/rvv/fixed-vectors-extract-i1.ll
68

It is my mistake, I thought things in wrong way. Here should not be in reversed order. I fixed it.

craig.topper added inline comments.Jan 17 2022, 10:02 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4323

elment -> element

4327

Sign extend means that bit 7 and above all have the same value. So why can't we just check bit 7?

craig.topper added inline comments.Jan 17 2022, 12:14 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4308

You don't need Zve to test this. Before Zve was added to the spec we added a command line option "-riscv-v-fixed-length-vector-elen-max" to clip the ELEN.

Address comment.

jacquesguan added inline comments.Jan 17 2022, 6:29 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4308

Done.

4323

Done.

4327

Done.

craig.topper added inline comments.Jan 17 2022, 9:18 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4302

Use unsigned. Nothing here needs 64-bits.

Address comment

jacquesguan added inline comments.Jan 17 2022, 10:53 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4302

Done.

craig.topper added inline comments.Jan 24 2022, 11:15 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4293–4294

This comment is out of date.

Address comment.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4293–4294

Done.

frasercrmck added inline comments.Jan 25 2022, 2:12 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4295

nit: Nums is a little unconventional. NumElts or something? You can also use getVectorNumElements because we know it's a fixed-length vector.

llvm/test/CodeGen/RISCV/rvv/fixed-vectors-extract-i1.ll
5

You'll need to rebase and swap +experimental-v for +v

272

This doesn't look like it's doing the right thing. We haven't modified the original extract index a1 at this point so we could be shifting an i32 right by up to 127 places? Have I missed something?

jacquesguan added inline comments.Jan 25 2022, 4:05 AM
llvm/test/CodeGen/RISCV/rvv/fixed-vectors-extract-i1.ll
272

Here we want to extract the (idx % 32)th bit from the GPR, and a1 owns the value of idx, so (idx % 32) is a1[4-0]. And because the shift instruction only uses the 0 - (log2(xlen)-1) bits of rs2, so we actually do not need to get a1[4-0], we could just use a1. So I think here is OK?

Address comment.

jacquesguan added inline comments.Jan 25 2022, 6:02 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4295

Done.

llvm/test/CodeGen/RISCV/rvv/fixed-vectors-extract-i1.ll
5

Done.

frasercrmck added inline comments.Jan 27 2022, 3:05 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4305

I think asserting that NumElts is a power of two (or just skipping this optimization) would be useful just in case we ever support other vector types. This is quite an edge case so won't be well covered, and could silently do some weird things.

llvm/test/CodeGen/RISCV/rvv/fixed-vectors-extract-i1.ll
272

Ah yes, that's what I missed. Thanks! I see there's an AND being generated anyway which should be doing the right thing.

Address comment.

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

Thanks, I add an assert for this.

frasercrmck accepted this revision.Jan 28 2022, 3:12 AM

LGTM, thanks

This revision is now accepted and ready to land.Jan 28 2022, 3:12 AM
This revision was landed with ongoing or failed builds.Jan 28 2022, 7:08 PM
This revision was automatically updated to reflect the committed changes.