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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


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


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


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


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


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


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


================
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);
----------------
I don't think you need to pass `false` here.


================
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);
----------------
Use SDValue instead of auto.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117389