* [PATCH 2/2] sim: riscv: Simplify the signed div by -1 code @ 2024-04-15 14:46 Bernd Edlinger 2024-06-11 16:25 ` Andrew Burgess 0 siblings, 1 reply; 4+ messages in thread From: Bernd Edlinger @ 2024-04-15 14:46 UTC (permalink / raw) To: gdb-patches This uses the idea from the previous patch to simplify the code for non-overflowing signed divisions by -1. This is no bug-fix but it simplifies the code and avoids some unnecessary branches. --- sim/riscv/sim-main.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c index 515ff835223..e4b15b533ba 100644 --- a/sim/riscv/sim-main.c +++ b/sim/riscv/sim-main.c @@ -700,18 +700,16 @@ execute_m (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) const char *rd_name = riscv_gpr_names_abi[rd]; const char *rs1_name = riscv_gpr_names_abi[rs1]; const char *rs2_name = riscv_gpr_names_abi[rs2]; - unsigned_word tmp, dividend_max; + unsigned_word tmp; sim_cia pc = riscv_cpu->pc + 4; - dividend_max = -((unsigned_word) 1 << (WITH_TARGET_WORD_BITSIZE - 1)); - switch (op->match) { case MATCH_DIV: TRACE_INSN (cpu, "div %s, %s, %s; // %s = %s / %s", rd_name, rs1_name, rs2_name, rd_name, rs1_name, rs2_name); - if (riscv_cpu->regs[rs1] == dividend_max && riscv_cpu->regs[rs2] == -1) - tmp = dividend_max; + if (riscv_cpu->regs[rs2] == -1) + tmp = -riscv_cpu->regs[rs1]; else if (riscv_cpu->regs[rs2]) tmp = (signed_word) riscv_cpu->regs[rs1] / (signed_word) riscv_cpu->regs[rs2]; @@ -793,7 +791,7 @@ execute_m (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) case MATCH_REM: TRACE_INSN (cpu, "rem %s, %s, %s; // %s = %s %% %s", rd_name, rs1_name, rs2_name, rd_name, rs1_name, rs2_name); - if (riscv_cpu->regs[rs1] == dividend_max && riscv_cpu->regs[rs2] == -1) + if (riscv_cpu->regs[rs2] == -1) tmp = 0; else if (riscv_cpu->regs[rs2]) tmp = (signed_word) riscv_cpu->regs[rs1] -- 2.39.2 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] sim: riscv: Simplify the signed div by -1 code 2024-04-15 14:46 [PATCH 2/2] sim: riscv: Simplify the signed div by -1 code Bernd Edlinger @ 2024-06-11 16:25 ` Andrew Burgess 2024-06-17 5:10 ` Bernd Edlinger 0 siblings, 1 reply; 4+ messages in thread From: Andrew Burgess @ 2024-06-11 16:25 UTC (permalink / raw) To: Bernd Edlinger, gdb-patches Bernd Edlinger <bernd.edlinger@hotmail.de> writes: > This uses the idea from the previous patch to > simplify the code for non-overflowing signed > divisions by -1. This is no bug-fix but it > simplifies the code and avoids some unnecessary > branches. > --- > sim/riscv/sim-main.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c > index 515ff835223..e4b15b533ba 100644 > --- a/sim/riscv/sim-main.c > +++ b/sim/riscv/sim-main.c > @@ -700,18 +700,16 @@ execute_m (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) > const char *rd_name = riscv_gpr_names_abi[rd]; > const char *rs1_name = riscv_gpr_names_abi[rs1]; > const char *rs2_name = riscv_gpr_names_abi[rs2]; > - unsigned_word tmp, dividend_max; > + unsigned_word tmp; > sim_cia pc = riscv_cpu->pc + 4; > > - dividend_max = -((unsigned_word) 1 << (WITH_TARGET_WORD_BITSIZE - 1)); I really don't understand what this was trying to calculate. It's not the max value of an unsigned_word. Nor is it the bit representation of the maximum signed_word value... but you're removing it, so I guess I can not worry about that :) > - > switch (op->match) > { > case MATCH_DIV: > TRACE_INSN (cpu, "div %s, %s, %s; // %s = %s / %s", > rd_name, rs1_name, rs2_name, rd_name, rs1_name, rs2_name); > - if (riscv_cpu->regs[rs1] == dividend_max && riscv_cpu->regs[rs2] == -1) > - tmp = dividend_max; > + if (riscv_cpu->regs[rs2] == -1) > + tmp = -riscv_cpu->regs[rs1]; As with the previous commit, can't we drop this whole if check and block? I think we should add some tests for this too. The test I proposed in the previous commit can be modified to give us div.s and rem.s tests. Thanks, Andrew > else if (riscv_cpu->regs[rs2]) > tmp = (signed_word) riscv_cpu->regs[rs1] / > (signed_word) riscv_cpu->regs[rs2]; > @@ -793,7 +791,7 @@ execute_m (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) > case MATCH_REM: > TRACE_INSN (cpu, "rem %s, %s, %s; // %s = %s %% %s", > rd_name, rs1_name, rs2_name, rd_name, rs1_name, rs2_name); > - if (riscv_cpu->regs[rs1] == dividend_max && riscv_cpu->regs[rs2] == -1) > + if (riscv_cpu->regs[rs2] == -1) > tmp = 0; > else if (riscv_cpu->regs[rs2]) > tmp = (signed_word) riscv_cpu->regs[rs1] > -- > 2.39.2 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] sim: riscv: Simplify the signed div by -1 code 2024-06-11 16:25 ` Andrew Burgess @ 2024-06-17 5:10 ` Bernd Edlinger 2024-06-17 8:58 ` Andrew Burgess 0 siblings, 1 reply; 4+ messages in thread From: Bernd Edlinger @ 2024-06-17 5:10 UTC (permalink / raw) To: Andrew Burgess, gdb-patches On 6/11/24 18:25, Andrew Burgess wrote: > Bernd Edlinger <bernd.edlinger@hotmail.de> writes: > >> This uses the idea from the previous patch to >> simplify the code for non-overflowing signed >> divisions by -1. This is no bug-fix but it >> simplifies the code and avoids some unnecessary >> branches. >> --- >> sim/riscv/sim-main.c | 10 ++++------ >> 1 file changed, 4 insertions(+), 6 deletions(-) >> >> diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c >> index 515ff835223..e4b15b533ba 100644 >> --- a/sim/riscv/sim-main.c >> +++ b/sim/riscv/sim-main.c >> @@ -700,18 +700,16 @@ execute_m (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) >> const char *rd_name = riscv_gpr_names_abi[rd]; >> const char *rs1_name = riscv_gpr_names_abi[rs1]; >> const char *rs2_name = riscv_gpr_names_abi[rs2]; >> - unsigned_word tmp, dividend_max; >> + unsigned_word tmp; >> sim_cia pc = riscv_cpu->pc + 4; >> >> - dividend_max = -((unsigned_word) 1 << (WITH_TARGET_WORD_BITSIZE - 1)); > > I really don't understand what this was trying to calculate. It's not > the max value of an unsigned_word. Nor is it the bit representation of > the maximum signed_word value... but you're removing it, so I guess I > can not worry about that :) > This is the largest negative dividend, either -2**31=-2147483648 or -2**63=-9223372036854775808 and dividing this value by any non-negative value is always fine, except for the divisor=-1, because the result will overflow, and can therefore cause problems. But by my reasoning I found a simple solution that handles the divisor -1 for all dividends correctly and more efficiently, by simply computing -(unsigned)x instead of x/-1. >> - >> switch (op->match) >> { >> case MATCH_DIV: >> TRACE_INSN (cpu, "div %s, %s, %s; // %s = %s / %s", >> rd_name, rs1_name, rs2_name, rd_name, rs1_name, rs2_name); >> - if (riscv_cpu->regs[rs1] == dividend_max && riscv_cpu->regs[rs2] == -1) >> - tmp = dividend_max; >> + if (riscv_cpu->regs[rs2] == -1) >> + tmp = -riscv_cpu->regs[rs1]; > > As with the previous commit, can't we drop this whole if check and block? > No in this case the division would indeed overflow on the division dividend_max/-1. > I think we should add some tests for this too. The test I proposed in > the previous commit can be modified to give us div.s and rem.s tests. > Well, since this is just refactoring, and there is already very good test coverage in the gcc testsuite, I would not do that here, but instead I think the overflow bug in the mulh instruction really deserves a test case: [PATCH] sim: riscv: Fix undefined behaviour in mulh and similar https://sourceware.org/pipermail/gdb-patches/2024-April/208616.html So I will post an update for this one with a test case that demonstrates the possible mis-compilation due to a compiler optimization based on the assumption that an undefined behaviour may never happen, which caused user visible effects in this case. Thanks Bernd. > Thanks, > Andrew > >> else if (riscv_cpu->regs[rs2]) >> tmp = (signed_word) riscv_cpu->regs[rs1] / >> (signed_word) riscv_cpu->regs[rs2]; >> @@ -793,7 +791,7 @@ execute_m (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) >> case MATCH_REM: >> TRACE_INSN (cpu, "rem %s, %s, %s; // %s = %s %% %s", >> rd_name, rs1_name, rs2_name, rd_name, rs1_name, rs2_name); >> - if (riscv_cpu->regs[rs1] == dividend_max && riscv_cpu->regs[rs2] == -1) >> + if (riscv_cpu->regs[rs2] == -1) >> tmp = 0; >> else if (riscv_cpu->regs[rs2]) >> tmp = (signed_word) riscv_cpu->regs[rs1] >> -- >> 2.39.2 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] sim: riscv: Simplify the signed div by -1 code 2024-06-17 5:10 ` Bernd Edlinger @ 2024-06-17 8:58 ` Andrew Burgess 0 siblings, 0 replies; 4+ messages in thread From: Andrew Burgess @ 2024-06-17 8:58 UTC (permalink / raw) To: Bernd Edlinger, gdb-patches Bernd Edlinger <bernd.edlinger@hotmail.de> writes: > On 6/11/24 18:25, Andrew Burgess wrote: >> Bernd Edlinger <bernd.edlinger@hotmail.de> writes: >> >>> This uses the idea from the previous patch to >>> simplify the code for non-overflowing signed >>> divisions by -1. This is no bug-fix but it >>> simplifies the code and avoids some unnecessary >>> branches. >>> --- >>> sim/riscv/sim-main.c | 10 ++++------ >>> 1 file changed, 4 insertions(+), 6 deletions(-) >>> >>> diff --git a/sim/riscv/sim-main.c b/sim/riscv/sim-main.c >>> index 515ff835223..e4b15b533ba 100644 >>> --- a/sim/riscv/sim-main.c >>> +++ b/sim/riscv/sim-main.c >>> @@ -700,18 +700,16 @@ execute_m (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) >>> const char *rd_name = riscv_gpr_names_abi[rd]; >>> const char *rs1_name = riscv_gpr_names_abi[rs1]; >>> const char *rs2_name = riscv_gpr_names_abi[rs2]; >>> - unsigned_word tmp, dividend_max; >>> + unsigned_word tmp; >>> sim_cia pc = riscv_cpu->pc + 4; >>> >>> - dividend_max = -((unsigned_word) 1 << (WITH_TARGET_WORD_BITSIZE - 1)); >> >> I really don't understand what this was trying to calculate. It's not >> the max value of an unsigned_word. Nor is it the bit representation of >> the maximum signed_word value... but you're removing it, so I guess I >> can not worry about that :) >> > > This is the largest negative dividend, either -2**31=-2147483648 or -2**63=-9223372036854775808 I guess my definition of 'max' and 'min' differ from whoever wrote this code! I'd describe those values as 'dividend_min'. But as the code is going away it's really not important. > and dividing this value by any non-negative value is always fine, except for the divisor=-1, > because the result will overflow, and can therefore cause problems. Ah, got you. This for sure should have a comment explaining the reasoning. > > But by my reasoning I found a simple solution that handles the divisor -1 for all dividends > correctly and more efficiently, by simply computing -(unsigned)x instead of x/-1. > >>> - >>> switch (op->match) >>> { >>> case MATCH_DIV: >>> TRACE_INSN (cpu, "div %s, %s, %s; // %s = %s / %s", >>> rd_name, rs1_name, rs2_name, rd_name, rs1_name, rs2_name); >>> - if (riscv_cpu->regs[rs1] == dividend_max && riscv_cpu->regs[rs2] == -1) >>> - tmp = dividend_max; >>> + if (riscv_cpu->regs[rs2] == -1) >>> + tmp = -riscv_cpu->regs[rs1]; >> >> As with the previous commit, can't we drop this whole if check and block? >> > > No in this case the division would indeed overflow on the division dividend_max/-1. Agree. Just add a comment explaining this please. > >> I think we should add some tests for this too. The test I proposed in >> the previous commit can be modified to give us div.s and rem.s tests. >> > > Well, since this is just refactoring, and there is already very good test > coverage in the gcc testsuite, I would not do that here, but instead The simulator should have tests in the simulator tree so I think we need to add tests. As I've already supplied a rough framework for the previous commit, which I believe you're going to polish for that test, all you need to do here is copy that framework, change divw to div, and fill in a few interesting test cases, so it shouldn't take long. > I think the overflow bug in the mulh instruction really deserves a test case: > > [PATCH] sim: riscv: Fix undefined behaviour in mulh and similar > https://sourceware.org/pipermail/gdb-patches/2024-April/208616.html > > So I will post an update for this one with a test case that demonstrates > the possible mis-compilation due to a compiler optimization based on the > assumption that an undefined behaviour may never happen, which caused > user visible effects in this case. Great, the more tests the better. Thanks, Andrew ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-06-17 8:58 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-04-15 14:46 [PATCH 2/2] sim: riscv: Simplify the signed div by -1 code Bernd Edlinger 2024-06-11 16:25 ` Andrew Burgess 2024-06-17 5:10 ` Bernd Edlinger 2024-06-17 8:58 ` Andrew Burgess
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).