[Bug middle-end/64182] New: [5 Regression] wide-int rounding division is broken

[Bug middle-end/64182] New: [5 Regression] wide-int rounding division is broken

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64182

            Bug ID: 64182
           Summary: [5 Regression] wide-int rounding division is broken
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rguenth at gcc dot gnu.org
                CC: rsandifo at gcc dot gnu.org

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.

[Bug middle-end/64182] [5 Regression] wide-int rounding division is broken

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64182

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1
   Target Milestone|---                         |5.0

[Bug middle-end/64182] [5 Regression] wide-int rounding division is broken

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64182

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 34222
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=34222&action=edit
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.

[Bug middle-end/64182] [5 Regression] wide-int rounding division is broken

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64182

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
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.

[Bug middle-end/64182] [5 Regression] wide-int rounding division is broken

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64182

rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2014-12-10
     Ever confirmed|0                           |1

--- Comment #3 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
(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.

[Bug middle-end/64182] [5 Regression] wide-int rounding division is broken

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64182

--- Comment #4 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
(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)

[Bug middle-end/64182] [5 Regression] wide-int rounding division is broken

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64182

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Makes sense.  Will you also fix double-int.c when you are on this?

[Bug middle-end/64182] [5 Regression] wide-int rounding division is broken

[zadeck at naturalbridge dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64182

Kenneth Zadeck <zadeck at naturalbridge dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |zadeck at naturalbridge dot com

--- Comment #7 from Kenneth Zadeck <zadeck at naturalbridge dot com> ---
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

[Bug middle-end/64182] [5 Regression] wide-int rounding division is broken

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64182

--- Comment #8 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
(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).

[Bug middle-end/64182] [5 Regression] wide-int rounding division is broken

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64182

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mpolacek at gcc dot gnu.org

--- Comment #10 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Fixed?

[Bug middle-end/64182] wide-int rounding division is broken

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64182

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[5 Regression] wide-int     |wide-int rounding division
                   |rounding division is broken |is broken

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think fixed on the trunk, but likely the double-int.c fix has not been
backported to release branches.

[Bug middle-end/64182] wide-int rounding division is broken

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64182

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|5.0                         |5.2

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 5.1 has been released.

[Bug middle-end/64182] wide-int rounding division is broken

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64182

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|5.2                         |5.3

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 5.2 is being released, adjusting target milestone to 5.3.