[PATCH] D53230: [RISCV] Introduce codegen patterns for RV64M-only instructions

[Bruce Hoult via Phabricator via llvm-commits]

brucehoult added inline comments.


================
Comment at: lib/Target/RISCV/RISCVInstrInfoM.td:57
+def : Pat<(sdiv (sexti32 GPR:$rs1), (sexti32 GPR:$rs2)),
+          (DIVW GPR:$rs1, GPR:$rs2)>;
+def : Pat<(sext_inreg (sdiv (sexti32 GPR:$rs1),
----------------
brucehoult wrote:
> efriedma wrote:
> > Are you sure `(sdiv (sexti32 GPR:$rs1), (sexti32 GPR:$rs2))` is sufficient?  I think it returns the wrong result for something like `INT_MIN/-1` (which should be a positive 64-bit value).
> > 
> > The pattern where the result is sign-extended is fine, I think.  And the corresponding "REMW" pattern is also fine.
> I'm not sure exactly what you're saying here.
> 
> A 32 bit INT_MIN/-1 which is 0x8000_0000/0xffffffff is defined to give INT_MIN 0x8000_0000 (which a DIVW will). On a 64 bit processor the end result will be 0xffff_ffff_8000_0000.
> 
> A 32 bit INT_MIN converted to 64 bit will be 0xffff_ffff_8000_0000, which when divided by a 64 bit -1 will give 0x0000_0001_0000_0000, which is incorrect for a 32 bit calculation.
> 
> Arguably, INT_MIN/-1 could have been defined to be 0 (the LSBs of the correct result), but it wasn't.
Oops ... that was wrong. Doing 0xffff_ffff_8000_0000 / 0xffff_ffff_ffff_ffff in 64 bit will give 0x0000_0000_8000_0000, which is correct if subsequently used or stored as a 32 bit value (though non-canonical), but it needs explicit sign extending if it will be used as a 64 bit value to be the same as the result of DIVW.



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

https://reviews.llvm.org/D53230