[PATCH] D59477: [RISCV] Custom lower SHL_PARTS, SRA_PARTS, SRL_PARTS

[Lewis Revill via Phabricator via llvm-commits]

lewis-revill added inline comments.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:557
+  //   Lo = Lo << (Shamt-XLEN)
+  //   Hi = (Hi << Shamt) | ((Lo >>u 1) >>u (XLEN-1 - Shamt))
+  // else:
----------------
I'm sure there's an off-by-one error in my reasoning here, but aren't we already safe to assume that `XLEN-Shamt` can fit in the bits available for srl? Since we checked for if `Shamt-XLEN < 0`, the only case where `XLEN-Shamt` couldn't fit would be where `XLEN-Shamt == XLEN` IE when `Shamt-XLEN == 0`, which is handled by the basic case below.

I'm assuming this was the reasoning for the `XLEN-1` stuff and the `(Lo >>u 1)` here, if not can someone explain it to me?


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:560
+  //   Lo = 0
+  //   Hi = Lo << Shamt
+
----------------
Nitpick: shouldn't this read `Hi = Lo << (Shamt-XLEN)`


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59477