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

[Jianjian Guan via Phabricator via llvm-commits]

jacquesguan added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4100
+    if (VecVT.isFixedLengthVector()) {
+      auto Nums = VecVT.getVectorMinNumElements();
+      if (Nums >= 8) {
----------------
craig.topper wrote:
> Use `unsigned` instead of auto
Done.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4102
+      if (Nums >= 8) {
+        MVT WideEleVT;
+        unsigned WidenVecLen;
----------------
craig.topper wrote:
> Ele->Elt would be a more common shortening.
Done.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4106
+        SDValue ExtractBitIdx;
+        if (Nums <= XLenVT.getScalarSizeInBits()) {
+          WideEleVT = MVT::getIntegerVT(Nums);
----------------
craig.topper wrote:
> getSizeBits(). XLenVT is always scalar.
Done


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4112
+        } else {
+          WideEleVT = XLenVT;
+          WidenVecLen = Nums / XLenVT.getScalarSizeInBits();
----------------
craig.topper wrote:
> This won't work with Zve32 on RV64. A vector XLen elements wouldn't be legal.
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`.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4113
+          WideEleVT = XLenVT;
+          WidenVecLen = Nums / XLenVT.getScalarSizeInBits();
+          ExtractElementIdx =
----------------
craig.topper wrote:
> getSizeBits(). XLenVT is always scalar.
Done


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4116
+              DAG.getNode(ISD::SRL, DL, XLenVT, Idx,
+                          DAG.getConstant(Log2_64(XLenVT.getScalarSizeInBits()),
+                                          DL, XLenVT));
----------------
craig.topper wrote:
> getSizeBits(). XLenVT is always scalar.
Done


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4118
+                                          DL, XLenVT));
+          ExtractBitIdx = DAG.getNode(
+              ISD::SHL, DL, XLenVT, Idx,
----------------
craig.topper wrote:
> This shift pair can be an AND right?
Done


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4133
+            ExtractBitIdx);
+        MVT WideVT = MVT::getVectorVT(WideEleVT, WidenVecLen, false);
+        Vec = DAG.getNode(ISD::BITCAST, DL, WideVT, Vec);
----------------
craig.topper wrote:
> I don't think you need to pass `false` here.
Done.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4135
+        Vec = DAG.getNode(ISD::BITCAST, DL, WideVT, Vec);
+        auto ExtractEle = DAG.getNode(ISD::EXTRACT_VECTOR_ELT, DL, XLenVT, Vec,
+                                      ExtractElementIdx);
----------------
craig.topper wrote:
> Use SDValue instead of auto.
Done.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-extract-i1.ll:68
+; RV32-NEXT:    li a0, 7
+; RV32-NEXT:    sub a0, a0, a1
+; RV32-NEXT:    vmv.x.s a1, v8
----------------
craig.topper wrote:
> I don't think I understand what this subtract is doing.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117389