Bug 64182 - wide-int rounding division is broken
Summary: wide-int rounding division is broken
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 5.0
: P1 normal
Target Milestone: 5.0
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2014-12-04 12:30 UTC by Richard Biener
Modified: 2017-07-27 05:55 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2014-12-10 00:00:00


Attachments
gcc5-pr64182.patch (381 bytes, patch)
2014-12-08 19:10 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Biener 2014-12-04 12:30:54 UTC
Testcase for gnat.dg/round_div.adb

-- { dg-do run }
-- { dg-options "-O3" }
procedure Round_Div is
   type Fixed is delta 1.0 range -2147483648.0 .. 2147483647.0;
   A : Fixed := 1.0;
   B : Fixed := 3.0;
   C : Integer;
   function Divide (X, Y : Fixed) return Integer is
   begin
      return Integer (X / Y);
   end;
begin
   C := Divide (A, B);
   if C /= 0 then
      raise Program_Error;
   end if;
end Round_Div;

Joseph pointed out round_mod_expr has the same issue.
Comment 1 Jakub Jelinek 2014-12-08 19:10:30 UTC
Created attachment 34222 [details]
gcc5-pr64182.patch

So like this (completely untested so far)?  I'm hoping that remainder can't be ever for signed numbers equal to minimum signed value and thus hopefully
wi::abs nor lshift of that by 1 should overflow.  For UNSIGNED, not sure if
wi::neg_p () is the right test for whether lshift by 1 will overflow.
Comment 2 Jakub Jelinek 2014-12-09 09:00:12 UTC
Note that double-int implementation looks broken too, it doesn't consider uns (negates both anyway if they are "negative"), and it probably doesn't handle the case of too big remainder either.
Comment 3 rsandifo@gcc.gnu.org 2014-12-10 14:51:41 UTC
(In reply to Jakub Jelinek from comment #1)
> Created attachment 34222 [details]
> gcc5-pr64182.patch
> 
> So like this (completely untested so far)?  I'm hoping that remainder can't
> be ever for signed numbers equal to minimum signed value and thus hopefully
> wi::abs nor lshift of that by 1 should overflow.  For UNSIGNED, not sure if
> wi::neg_p () is the right test for whether lshift by 1 will overflow.

Looks OK to me, but I wonder if we could just use:

  wi::geu_p (y, remainder - y)

for unsigned, and similarly with abses for signed, which avoids having to worry about overflow.  Will try.
Comment 4 rsandifo@gcc.gnu.org 2014-12-10 14:52:39 UTC
(In reply to rsandifo@gcc.gnu.org from comment #3)
> (In reply to Jakub Jelinek from comment #1)
> > Created attachment 34222 [details]
> > gcc5-pr64182.patch
> > 
> > So like this (completely untested so far)?  I'm hoping that remainder can't
> > be ever for signed numbers equal to minimum signed value and thus hopefully
> > wi::abs nor lshift of that by 1 should overflow.  For UNSIGNED, not sure if
> > wi::neg_p () is the right test for whether lshift by 1 will overflow.
> 
> Looks OK to me, but I wonder if we could just use:
> 
>   wi::geu_p (y, remainder - y)
> 
> for unsigned, and similarly with abses for signed, which avoids having to
> worry about overflow.  Will try.

Er, of course I mean:

   wi::geu_p (remainder, y - remainder)
Comment 5 Jakub Jelinek 2014-12-10 15:27:34 UTC
Makes sense.  Will you also fix double-int.c when you are on this?
Comment 6 rsandifo@gcc.gnu.org 2014-12-10 16:42:02 UTC
(In reply to Jakub Jelinek from comment #5)
> Makes sense.  Will you also fix double-int.c when you are on this?

OK, I'll take the same approach there.  I don't know how to test that
code "properly", so in the end I just did some double_int arithmetic
at the start of main().  Like you say it wasn't rounding correctly
in the unsigned case.
Comment 7 Kenneth Zadeck 2014-12-12 00:58:54 UTC
I do believe that it is possible to test the double int code if you wrote a program that used fixed point math.    I have never used fixed point math and have no interest in doing so, but double it is the implementation platform for fixed point math.

kenny
Comment 8 rsandifo@gcc.gnu.org 2014-12-12 07:52:54 UTC
(In reply to Kenneth Zadeck from comment #7)
> I do believe that it is possible to test the double int code if you wrote a
> program that used fixed point math.    I have never used fixed point math
> and have no interest in doing so, but double it is the implementation
> platform for fixed point math.

Yeah.  Richi pointed out in the review that we could just use a GCC plugin to test it, so I've got a patch for that going through internal review.  Using a plugin is much nicer because it doesn't depend on a non-default language or on fixed-point support.  It also means we can test the wi:: routines directly (in addition to the Ada testcase, which I'll still commit).
Comment 9 rsandifo@gcc.gnu.org 2014-12-12 15:47:29 UTC
Author: rsandifo
Date: Fri Dec 12 15:46:57 2014
New Revision: 218678

URL: https://gcc.gnu.org/viewcvs?rev=218678&root=gcc&view=rev
Log:
gcc/
	PR middle-end/64182
	* wide-int.h (wi::div_round, wi::mod_round): Fix rounding of tied
	cases.
	* double-int.c (div_and_round_double): Fix handling of unsigned
	cases.  Use same rounding approach as wide-int.h.

gcc/testsuite/
2014-xx-xx  Richard Sandiford  <richard.sandiford@arm.com>
	    Joseph Myers  <joseph@codesourcery.com>

	PR middle-end/64182
	* gcc.dg/plugin/wide-int-test-1.c,
	gcc.dg/plugin/wide-int_plugin.c: New test.
	* gcc.dg/plugin/plugin.exp: Register it.
	* gnat.dg/round_div.adb: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/plugin/wide-int-test-1.c
    trunk/gcc/testsuite/gcc.dg/plugin/wide-int_plugin.c
    trunk/gcc/testsuite/gnat.dg/round_div.adb
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/double-int.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.dg/plugin/plugin.exp
    trunk/gcc/wide-int.h
Comment 10 Marek Polacek 2014-12-17 12:39:20 UTC
Fixed?
Comment 11 Jakub Jelinek 2015-01-05 10:13:14 UTC
I think fixed on the trunk, but likely the double-int.c fix has not been backported to release branches.
Comment 12 Jakub Jelinek 2015-04-22 11:58:12 UTC
GCC 5.1 has been released.
Comment 13 Richard Biener 2015-07-16 09:11:10 UTC
GCC 5.2 is being released, adjusting target milestone to 5.3.
Comment 14 Richard Biener 2015-12-04 10:43:30 UTC
GCC 5.3 is being released, adjusting target milestone.
Comment 15 Richard Biener 2016-06-03 10:03:54 UTC
GCC 5.4 is being released, adjusting target milestone.
Comment 16 Andrew Pinski 2017-07-27 05:55:15 UTC
Fixed for GCC 5.