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

[Luís Marques via Phabricator via llvm-commits]

luismarques 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:
----------------
lewis-revill wrote:
> lewis-revill wrote:
> > lewis-revill wrote:
> > > 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?
> > I see at least part of my error. It should read 'IE when `Shamt == 0``. Either way I'm not seeing how it should come up?
> Okay sorry for the noise, I see why this is an issue now. `Shamt == 0` is indeed handled by this case. Could we have some comment to that effect in this function? And likewise for `lowerShiftRightParts`?
I think the comment "// Shamt < XLEN" documents reasonably well that this branch also covers Shamt == 0, since "0 < XLEN". If I misunderstood your comment please clarify.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:560
+  //   Lo = 0
+  //   Hi = Lo << Shamt
+
----------------
lewis-revill wrote:
> Nitpick: shouldn't this read `Hi = Lo << (Shamt-XLEN)`
The (Shamt-XLEN) should have been in the "else" branch, not the "then" branch. These kind of mistakes were easy to make because the assembly has the "basic blocks" in the reverse order (I can't fix that in the pseudo-code without inverting the condition, which would be even more confusing). I had caught most of them, but that one slipped through. Good catch! I'll fix that together with the remaining issues.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59477