calculate overflow type in wide int arithmetic

[Aldy Hernandez]

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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: curr.patch
Type: text/x-patch
Size: 55104 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20180706/c3b418b1/attachment.bin>