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

[Alex Bradbury via Phabricator via llvm-commits]

asb marked an inline comment as done.
asb 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),
----------------
efriedma wrote:
> brucehoult wrote:
> > 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.
> > 
> The overall point is that I think the pattern miscompiles the following: `int64_t f(int32_t x, int32_t y) { return (int64_t)x/y; }`.
Thanks for the catch, I agree this pattern is incorrect. I had reasoned that i32 INT_MIN/-1 is undefined behaviour. As you rightly point out, there's no guarantee that the sdiv started life as an i32 sdiv (as opposed to an i64 sdiv that happens to have sexti32 operands) - so this is basically the same problem we've discussed before.


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

https://reviews.llvm.org/D53230