[PATCH] D120899: [RISCV] Fix vslide1up/down intrinsics overflow bug for SEW=64 on RV32

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4657
+        unsigned KnownSize = VT.getSizeInBits().getKnownMinValue();
+        // get the smallest vlmax (vector register size is 128 bits)
+        VLMAX1 = 128 / 64 * KnownSize / 64;
----------------
Where does 128 come from? The vector extension allows 32-bit for Zve32* and 64-bit for Zve64*


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4658
+        // get the smallest vlmax (vector register size is 128 bits)
+        VLMAX1 = 128 / 64 * KnownSize / 64;
+        // get the largest vlmax (vector register size is 65536 bits)
----------------
I'm not sure I understand this math. What is 64?

KnownSize can be as small as 8 so this returns 0 in that case.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4662
+      }
+      auto *C = dyn_cast<ConstantSDNode>(AVL);
+      unsigned AVLInt = C->getZExtValue();
----------------
Use cast instead of dyn_cast. dyn_cast returns null if the cast fails and that would need to be checked, but you already called `isa` earlier, so it can't fail.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4663
+      auto *C = dyn_cast<ConstantSDNode>(AVL);
+      unsigned AVLInt = C->getZExtValue();
+      if (AVLInt <= VLMAX1) {
----------------
AVL is a 64-bit value on RV64 so you can't use unsigned here.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4666
+        I32VL = DAG.getConstant(AVLInt << 1, DL, XLenVT);
+      } else if (AVLInt >= VLMAX2 << 1) {
+        I32VL = DAG.getConstant(VLMAX2 << 1, DL, XLenVT);
----------------
Use `* 2` instead of `<< 1`.


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

https://reviews.llvm.org/D120899