This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: calculate overflow type in wide int arithmetic


Richard Biener <richard.guenther@gmail.com> writes:
> On Fri, Jul 6, 2018 at 9:50 AM Aldy Hernandez <aldyh@redhat.com> wrote:
>>
>>
>>
>> On 07/05/2018 05:50 AM, Richard Biener wrote:
>> > On Thu, Jul 5, 2018 at 9:35 AM Aldy Hernandez <aldyh@redhat.com> wrote:
>> >>
>> >> The reason for this patch are the changes showcased in tree-vrp.c.
>> >> Basically I'd like to discourage rolling our own overflow and underflow
>> >> calculation when doing wide int arithmetic.  We should have a
>> >> centralized place for this, that is-- in the wide int code itself ;-).
>> >>
>> >> The only cases I care about are plus/minus, which I have implemented,
>> >> but we also get division for free, since AFAICT, division can only
>> >> positive overflow:
>> >>
>> >>          -MIN / -1 => +OVERFLOW
>> >>
>> >> Multiplication OTOH, can underflow, but I've not implemented it because
>> >> we have no uses for it.  I have added a note in the code explaining this.
>> >>
>> >> Originally I tried to only change plus/minus, but that made code that
>> >> dealt with plus/minus in addition to div or mult a lot uglier.  You'd
>> >> have to special case "int overflow_for_add_stuff" and "bool
>> >> overflow_for_everything_else".  Changing everything to int, makes things
>> >> consistent.
>> >>
>> >> Note: I have left poly-int as is, with its concept of yes/no for
>> >> overflow.  I can adapt this as well if desired.
>> >>
>> >> Tested on x86-64 Linux.
>> >>
>> >> OK for trunk?
>> >
>> > looks all straight-forward but the following:
>> >
>> >     else if (op1)
>> >       {
>> >         if (minus_p)
>> > -       {
>> > -         wi = -wi::to_wide (op1);
>> > -
>> > -         /* Check for overflow.  */
>> > -         if (sgn == SIGNED
>> > -             && wi::neg_p (wi::to_wide (op1))
>> > -             && wi::neg_p (wi))
>> > -           ovf = 1;
>> > -         else if (sgn == UNSIGNED && wi::to_wide (op1) != 0)
>> > -           ovf = -1;
>> > -       }
>> > +       wi = wi::neg (wi::to_wide (op1));
>> >         else
>> >          wi = wi::to_wide (op1);
>> >
>> > you fail to handle - -INT_MIN.
>>
>> Woah, very good catch.  I previously had this implemented as wi::sub(0,
>> op1, &ovf) which was calculating overflow correctly but when I
>> implemented the overflow type in wi::neg I missed this.  Thanks.
>>
>> >
>> > Given the fact that for multiplication (or others, didn't look too  close)
>> > you didn't implement the direction indicator I wonder if it would be more
>> > appropriate to do
>> >
>> > enum ovfl { OVFL_NONE = 0, OVFL_UNDERFLOW = -1, OVFL_OVERFLOW = 1,
>> > OVFL_UNKNOWN = 2 };
>> >
>> > and tell us the "truth" here?
>>
>> Excellent idea...though it came with lots of typing :).  Fixed.
>>
>> BTW, if I understand correctly, I've implemented the overflow types
>> correctly for everything but multiplication (which we have no users for
>> and I return OVF_UNKNOWN).  I have indicated this in comments.  Also,
>> for division I did nothing special, as we can only +OVERFLOW.
>>
>> >
>> > Hopefully if (overflow) will still work with that.
>>
>> It does.
>>
>> >
>> > Otherwise can you please add a toplevel comment to wide-int.h as to what the
>> > overflow result semantically is for a) SIGNED and b) UNSIGNED operations?
>>
>> Done.  Let me know if the current comment is what you had in mind.
>>
>> OK for trunk?
>
> I'd move accumulate_overflow to wi::, it looks generally useful.  That function
> misses to handle the !suboverflow && overflow case optimally.
>
> I see that poly-int choses to accumulate overflow (callers need to
> initialize it) while wide_int chooses not to accumulate...  to bad
> this is inconsistent.  Richard?

poly-int needs to accumulate internally when handling multiple coefficients,
but the external interface is the same as for wi:: (no caller initialisation).

Richard


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]