[PATCH] D117389: [RISCV] Improve extract_vector_elt for fixed mask registers.

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4270
+        if (NumElts <= LargestEltVT.getSizeInBits()) {
+          WideEltVT = MVT::getIntegerVT(NumElts);
+          WidenVecLen = 1;
----------------
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.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-extract-i1.ll:272
+; RV32-NEXT:    vmv.x.s a0, v8
+; RV32-NEXT:    srl a0, a0, a1
+; RV32-NEXT:    andi a0, a0, 1
----------------
jacquesguan wrote:
> frasercrmck wrote:
> > 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?
> 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?
Ah yes, that's what I missed. Thanks! I see there's an `AND` being generated anyway which should be doing the right thing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117389/new/

https://reviews.llvm.org/D117389