On Fri, Jul 6, 2018 at 9:50 AM Aldy Hernandez <al...@redhat.com> wrote:
>
>
>
> On 07/05/2018 05:50 AM, Richard Biener wrote:
> > On Thu, Jul 5, 2018 at 9:35 AM Aldy Hernandez <al...@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?
OK with the fix/move of accumulate_overflow.
Thanks,
Richard.
