Hi Aldy,
> 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?
this patch broke SPARC bootstrap:
/vol/gcc/src/hg/trunk/local/gcc/config/sparc/sparc.c: In function 'tree_node*
sparc_fold_builtin(tree, int, tree_node**, bool)':
/vol/gcc/src/hg/trunk/local/gcc/config/sparc/sparc.c:11868:52: error: no
matching function for call to 'neg(wi::tree_to_widest_ref, bool*)'
tmp = wi::neg (wi::to_widest (e1), &neg1_ovf);
^
In file included from /vol/gcc/src/hg/trunk/local/gcc/coretypes.h:399:0,
from /vol/gcc/src/hg/trunk/local/gcc/config/sparc/sparc.c:27:
/vol/gcc/src/hg/trunk/local/gcc/wide-int.h:2127:1: note: candidate:
template<class T> typename wi::binary_traits<T, T>::result_type wi::neg(const
T&)
wi::neg (const T &x)
^~
/vol/gcc/src/hg/trunk/local/gcc/wide-int.h:2127:1: note: template argument
deduction/substitution failed:
/vol/gcc/src/hg/trunk/local/gcc/config/sparc/sparc.c:11868:52: note:
candidate expects 1 argument, 2 provided
tmp = wi::neg (wi::to_widest (e1), &neg1_ovf);
^
In file included from /vol/gcc/src/hg/trunk/local/gcc/coretypes.h:399:0,
from /vol/gcc/src/hg/trunk/local/gcc/config/sparc/sparc.c:27:
/vol/gcc/src/hg/trunk/local/gcc/wide-int.h:2136:1: note: candidate:
template<class T> typename wi::binary_traits<T, T>::result_type wi::neg(const
T&, wi::overflow_type*)
wi::neg (const T &x, overflow_type *overflow)
^~
/vol/gcc/src/hg/trunk/local/gcc/wide-int.h:2136:1: note: template argument
deduction/substitution failed:
/vol/gcc/src/hg/trunk/local/gcc/config/sparc/sparc.c:11868:43: note: cannot
convert '& neg1_ovf' (type 'bool*') to type 'wi::overflow_type*'
tmp = wi::neg (wi::to_widest (e1), &neg1_ovf);
^~~~~~~~~
In file included from /vol/gcc/src/hg/trunk/local/gcc/coretypes.h:414:0,
from /vol/gcc/src/hg/trunk/local/gcc/config/sparc/sparc.c:27:
/vol/gcc/src/hg/trunk/local/gcc/poly-int.h:1052:1: note: candidate:
template<unsigned int N, class Ca> poly_int<N, typename wi::binary_traits<T2,
T2>::result_type> wi::neg(const poly_int_pod<N, C>&)
neg (const poly_int_pod<N, Ca> &a)
^~~
/vol/gcc/src/hg/trunk/local/gcc/poly-int.h:1052:1: note: template argument
deduction/substitution failed:
/vol/gcc/src/hg/trunk/local/gcc/config/sparc/sparc.c:11868:52: note:
'wi::tree_to_widest_ref {aka const generic_wide_int<wi::extended_tree<192> >}'
is not derived from 'const poly_int_pod<N, C>'
tmp = wi::neg (wi::to_widest (e1), &neg1_ovf);
^
In file included from /vol/gcc/src/hg/trunk/local/gcc/coretypes.h:414:0,
from /vol/gcc/src/hg/trunk/local/gcc/config/sparc/sparc.c:27:
/vol/gcc/src/hg/trunk/local/gcc/poly-int.h:1063:1: note: candidate:
template<unsigned int N, class Ca> poly_int<N, typename wi::binary_traits<T2,
T2>::result_type> wi::neg(const poly_int_pod<N, C>&, wi::overflow_type*)
neg (const poly_int_pod<N, Ca> &a, wi::overflow_type *overflow)
^~~
/vol/gcc/src/hg/trunk/local/gcc/poly-int.h:1063:1: note: template argument
deduction/substitution failed:
/vol/gcc/src/hg/trunk/local/gcc/config/sparc/sparc.c:11868:52: note:
'wi::tree_to_widest_ref {aka const generic_wide_int<wi::extended_tree<192> >}'
is not derived from 'const poly_int_pod<N, C>'
tmp = wi::neg (wi::to_widest (e1), &neg1_ovf);
^
and several more. This seems to be the only backend that uses the
additional bool * argument to wi::neg etc.
Fixed as follows, bootstrapped on sparc-sun-solaris2.11.
Ok for mainline?
Rainer
--
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University
2018-07-09 Rainer Orth <r...@cebitec.uni-bielefeld.de>
* config/sparc/sparc.c (sparc_fold_builtin) <SPARC_BUILTIN_PDIST,
SPARC_BUILTIN_PDISTN>: Adapt for signature change of wi::neg,
wi::add.
# HG changeset patch
# Parent 616e203d3f529c24fda5ee74a06da11904d74ddb
Fix overflow handling in sparc.c:sparc_fold_builtin
diff --git a/gcc/config/sparc/sparc.c b/gcc/config/sparc/sparc.c
--- a/gcc/config/sparc/sparc.c
+++ b/gcc/config/sparc/sparc.c
@@ -11863,16 +11863,19 @@ sparc_fold_builtin (tree fndecl, int n_a
tree e0 = VECTOR_CST_ELT (arg0, i);
tree e1 = VECTOR_CST_ELT (arg1, i);
- bool neg1_ovf, neg2_ovf, add1_ovf, add2_ovf;
+ wi::overflow_type neg1_ovf, neg2_ovf, add1_ovf, add2_ovf;
tmp = wi::neg (wi::to_widest (e1), &neg1_ovf);
tmp = wi::add (wi::to_widest (e0), tmp, SIGNED, &add1_ovf);
if (wi::neg_p (tmp))
tmp = wi::neg (tmp, &neg2_ovf);
else
- neg2_ovf = false;
+ neg2_ovf = wi::OVF_NONE;
result = wi::add (result, tmp, SIGNED, &add2_ovf);
- overflow |= neg1_ovf | neg2_ovf | add1_ovf | add2_ovf;
+ overflow |= ((neg1_ovf != wi::OVF_NONE)
+ | (neg2_ovf != wi::OVF_NONE)
+ | (add1_ovf != wi::OVF_NONE)
+ | (add2_ovf != wi::OVF_NONE));
}
gcc_assert (!overflow);
