[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