[PATCH] D119759: [RISCV] Match shufflevector corresponding to slideup.

Zakk Chen via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Feb 16 07:47:55 PST 2022


khchen added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2490
 
-static int isElementRotate(SDValue &V1, SDValue &V2, ArrayRef<int> Mask) {
+static int isElementRotate(int &Lo, int &Hi, ArrayRef<int> Mask) {
   int Size = Mask.size();
----------------
nit: I feel it would be good to have a comment talk about what's number mean for `L0` and `Hi`.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2545
   assert(Rotation != 0 && "Failed to locate a viable rotation!");
-  assert((Lo || Hi) && "Failed to find a rotated input vector!");
-
-  // Make sure we've found a value for both halves.
-  if (!Lo || !Hi)
-    return -1;
-
-  V1 = Lo;
-  V2 = Hi;
+  assert((Lo >= 0 || Hi >= 0) && "Failed to find a rotated input vector!");
 
----------------
If I understand code correctly, the `Lo` and `Hi` could be either 0 or 1, why do we need to check `>=0` for them?
same question for line 2649 and 2651.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2658
+    if (LoV)
+      LoV = convertToScalableVector(ContainerVT, LoV, DAG, Subtarget);
+    if (HiV)
----------------
Why not move this line into 2650? is it because readability?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2670
+      SDValue DownVL = VL;
+      // If we are doing a SLIDEDOWN+SLIDEUP, reduce the VL for the SLIDEDOWN.
+      if (LoV)
----------------
nit: I feel a little bit confusing here because we have already mentioned this behavior above but using different words.

```
// We found a rotation. We need to slide V1 down by Rotation. Using
// (NumElts - Rotation) for VL. Then we need to slide V2 up by
// (NumElts - Rotation) using NumElts for VL.
```

If we are doing SLIDEDOWN+SLIDEUP (alias to rotation), reduce the VL (alias reduce VL into to "NumElts - Rotation") for the SLIDEDOWN.

In addition, I think 2654~2656 comment need to replace `V1` and `V2` with `LoV` and `HiV`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119759



More information about the llvm-commits mailing list