RISC-V: align RISC-V software division with hardware specification in case of division by zero

Message ID 6396c774e3cc4378808d2a88f4051fa6@csem.ch
State New
Headers show
Series
  • RISC-V: align RISC-V software division with hardware specification in case of division by zero
Related show

Commit Message

Kewen.Lin via Gcc-patches May 29, 2020, 8:53 a.m.
The assembly code in libgcc/config/riscv/div.S does not handle the division by zero as specified in the riscv-spec v2.2 chapter 6.2 in case of signed division:

"The quotient of division by zero has all bits set, i.e. 2XLEN−1 for unsigned division or−1 for signed division."

When a negative number is divided by zero in the __divdi3 function, the result is 1 instead of -1.

As soon as there exists a specific implementation for the software division for the RISC-V and although that the C language allows unspecified result in case of division by zero, it would be worth aligning the software RISC-V division with the hardware implementation so that the compliance tests could pass whatever the -mno-div or -mdiv flag value.
Especially in the case where the patch is simple, does not add additional code size nor execution time.

The patch proposes that when the dividend is negative, a zero divisor is considered as negative so that the result of the unsigned division will not be inverted. This consists of exchanging a "branch greater or equal zero (bgez)" with a "branch greater than zero (bgtz)".

Virginie

-------------------------------------------

ChangeLog

*libgcc/config/riscv/div.S: Change handling of signed division by zero in case of negative dividend to align with RISC-V specification for hardware division.

--------------------------------------------

Comments

Jim Wilson June 2, 2020, 6:46 p.m. | #1
On Fri, May 29, 2020 at 1:53 AM MOSER Virginie via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
> The assembly code in libgcc/config/riscv/div.S does not handle the division by zero as specified in the riscv-spec v2.2 chapter 6.2 in case of signed division:


This looks OK.  There are some administrative comments to make here.

FSF policy requires copyright assignments.  I don't see one for you or
your employer.  If you are serious about contributing to GNU projects.
you need to get a copyright assignment.  This patch is small enough
that I can accept it, but I may not be able to accept the next one.
If you don't get a copyright assignment, you might consider filing bug
reports instead and let someone with an assignment write the patch.

The patch is mime encoded which is inconvenient.  The patch also got
corrupted somehow.  I had to edit it before I could get it to apply.
Or maybe the patch was for an older gcc version?  git format-patch and
git send-email is a good way to create and send a patch.  Or you can
file a bug report and add the patch as an attachment there.

ChangeLog entries should describe what change was made.  Not why it
was made.  The why should go into the git commit or comments in the
code.  I rewrote the ChangeLog entry.

We prefer lines that are less than 80 characters.  I moved the comment
to a separate line and also fixed some other nearby lines.  Comments
end with a period and two spaces.

I fixed up the patch, tested it, and committed it.  I will send the
patch I committed.

Jim

Patch

diff --git a/libgcc/config/riscv/div.S b/libgcc/config/riscv/div.S
index 922a4338042..57f5856e11d 100644
--- a/libgcc/config/riscv/div.S
+++ b/libgcc/config/riscv/div.S
@@ -107,7 +107,8 @@  FUNC_END (__umoddi3)
   /* Handle negative arguments to __divdi3.  */
.L10:
   neg   a0, a0
-  bgez  a1, .L12      /* Compute __udivdi3(-a0, a1), then negate the result.  */
+  bgtz  a1, .L12      /* Compute __udivdi3(-a0, a1), then negate the result.  */
+                      /* Zero is handled as a negative so that the result will not be inverted */
   neg   a1, a1
   j     __udivdi3     /* Compute __udivdi3(-a0, -a1).  */