* [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; 5+ 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] 5+ 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; 5+ 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] 5+ 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; 5+ 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] 5+ 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 2024-06-17 10:43 ` Bernd Edlinger 0 siblings, 1 reply; 5+ 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] 5+ messages in thread
* Re: [PATCH 2/2] sim: riscv: Simplify the signed div by -1 code 2024-06-17 8:58 ` Andrew Burgess @ 2024-06-17 10:43 ` Bernd Edlinger 0 siblings, 0 replies; 5+ messages in thread From: Bernd Edlinger @ 2024-06-17 10:43 UTC (permalink / raw) To: Andrew Burgess, gdb-patches On 6/17/24 10:58, Andrew Burgess wrote: > 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. > Ack. >> 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. > Okay, I'll add this comment: --- a/sim/riscv/sim-main.c +++ b/sim/riscv/sim-main.c @@ -766,6 +766,9 @@ execute_m (SIM_CPU *cpu, unsigned_word iw, const struct riscv_opcode *op) 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); + /* Dividing by -1 can potentially overflow, and has therefore + to be done separately. Note that the sign change operation + is done on an unsigned value and is therefore safe to do. */ if (riscv_cpu->regs[rs2] == -1) tmp = -riscv_cpu->regs[rs1]; else if (riscv_cpu->regs[rs2]) How do you like that explanation? >> >>> 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. > Okay, if you like, I will add some more tests, the only interesting thing is how they made the result of divide by zero defined, but this patch did not touch that code, yet it may be worth to have some test coverage for that else branch here. Thanks Bernd. >> 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] 5+ messages in thread
end of thread, other threads:[~2024-06-17 10:41 UTC | newest] Thread overview: 5+ 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 2024-06-17 10:43 ` Bernd Edlinger
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).