diff mbox

Some wide-int review comments

Message ID 5294CD41.1040008@naturalbridge.com
State New
Headers show

Commit Message

Kenneth Zadeck Nov. 26, 2013, 4:33 p.m. UTC 
Richi,

patch ping

also two more pieces of information.    With further testing, this seems 
to fix

Tests that now work, but didn't before:

================
On 11/20/2013 06:31 PM, Kenneth Zadeck wrote:
> On 11/13/2013 04:59 AM, Richard Biener wrote:
>> On Tue, Nov 12, 2013 at 5:43 PM, Kenneth Zadeck
>> <zadeck@naturalbridge.com> wrote:
>>> On 11/12/2013 11:27 AM, Joseph S. Myers wrote:
>>>> On Tue, 12 Nov 2013, Kenneth Zadeck wrote:
>>>>
>>>>> Richi,
>>>>>
>>>>> i am having a little trouble putting this back the way that you want.
>>>>> The
>>>>> issue is rem.
>>>>> what is supposed to happen for INT_MIN % -1?
>>>>>
>>>>> I would assume because i am failing the last case of
>>>>> gcc.dg/c90-const-expr-8.c
>>>>> that INT_MIN %-1 should not overflow even if INT_MIN / -1 does.
>>>>> however,
>>>> Given the conclusion in C11 that a%b should be considered undefined 
>>>> if a/b
>>>> is not representable, I think it's reasonable to say INT_MIN % -1 
>>>> *should*
>>>> be considered to overflow (for all C standard versions) (and bug 
>>>> 30484 is
>>>> only a bug for -fwrapv).
>>>>
>>> however, my local question is what do we want the api to be
>>> int-const-binop-1?    The existing behavior seems to be that at 
>>> least for
>>> common modes this function silently returns 0 and it is up to the 
>>> front ends
>>> to put their own spin on it.
>> For wide-int you create 1:1 the behavior of current trunk (if a 
>> change of
>> behavior in TImode is not tested in the testsuite then you can ignore 
>> that).
>> Whatever change you do to semantics of functions you do separately
>> from wide-int (preferably first on trunk, or at your choice after the 
>> wide-int
>> merge).
>>
>> For this case in question I'd say a % -1 should return 0, but for
>> INT_MIN % -1 that 0 should have TREE_OVERFLOW set (and
>> thus you need to adjust that c90-const-expr-8.c testcase).
>>
>> Richard.
>>
>>> kenny
> richi,
> I have done this exactly as you suggested.   bootstrapped and 
> regression tested on x86-64.
>
> 2013-11-20  Kenneth Zadeck  <zadeck@naturalbridge.com>
>
>     * fold-const.c
>     (int_const_binop_1): Make INT_MIN % -1 return 0 with the overflow
>     bit set.
>
>
> 2013-11-20  Kenneth Zadeck  <zadeck@naturalbridge.com>
>
>     * gcc.dg/c90-const-expr-8.c: Look for overflow on INT_MIN % -1.
>     * gcc.dg/c99-const-expr-8.c: Look for overflow on INT_MIN % -1.
>
> ok to commit?
>
> kenny
>

Comments

Richard Biener Nov. 27, 2013, 10:50 a.m. UTC | #1 
On Tue, Nov 26, 2013 at 5:33 PM, Kenneth Zadeck
<zadeck@naturalbridge.com> wrote:
> Richi,
>
> patch ping

Ok.

Thanks,
Richard.

> also two more pieces of information.    With further testing, this seems to
> fix
>
> Tests that now work, but didn't before:
> ===============
> ext/random/hypergeometric_distribution/operators/values.cc (test for excess
> errors)
>
> New tests that PASS:
>
> ext/random/hypergeometric_distribution/operators/values.cc execution test
> ================
>
> also, the corresponding frag for fold-const.c on the wide-int branch will
> look like
> ================
> Index: gcc/fold-const.c
> ===================================================================
> --- gcc/fold-const.c    (revision 205224)
> +++ gcc/fold-const.c    (working copy)
> @@ -1030,51 +1030,51 @@ int_const_binop_1 (enum tree_code code,
>
>      case TRUNC_DIV_EXPR:
>      case EXACT_DIV_EXPR:
> -      res = wi::div_trunc (arg1, arg2, sign, &overflow);
> -      if (overflow)
> +      if (arg2 == 0)
>      return NULL_TREE;
> +      res = wi::div_trunc (arg1, arg2, sign, &overflow);
>        break;
>
>      case FLOOR_DIV_EXPR:
> -      res = wi::div_floor (arg1, arg2, sign, &overflow);
> -      if (overflow)
> +      if (arg2 == 0)
>      return NULL_TREE;
> +      res = wi::div_floor (arg1, arg2, sign, &overflow);
>        break;
>
>      case CEIL_DIV_EXPR:
> -      res = wi::div_ceil (arg1, arg2, sign, &overflow);
> -      if (overflow)
> +      if (arg2 == 0)
>      return NULL_TREE;
> +      res = wi::div_ceil (arg1, arg2, sign, &overflow);
>        break;
>
>      case ROUND_DIV_EXPR:
> -      res = wi::div_round (arg1, arg2, sign, &overflow);
> -      if (overflow)
> +      if (arg2 == 0)
>      return NULL_TREE;
> +      res = wi::div_round (arg1, arg2, sign, &overflow);
>        break;
>
>      case TRUNC_MOD_EXPR:
> -      res = wi::mod_trunc (arg1, arg2, sign, &overflow);
> -      if (overflow)
> +      if (arg2 == 0)
>      return NULL_TREE;
> +      res = wi::mod_trunc (arg1, arg2, sign, &overflow);
>        break;
>
>      case FLOOR_MOD_EXPR:
> -      res = wi::mod_floor (arg1, arg2, sign, &overflow);
> -      if (overflow)
> +      if (arg2 == 0)
>      return NULL_TREE;
> +      res = wi::mod_floor (arg1, arg2, sign, &overflow);
>        break;
>
>      case CEIL_MOD_EXPR:
> -      res = wi::mod_ceil (arg1, arg2, sign, &overflow);
> -      if (overflow)
> +      if (arg2 == 0)
>      return NULL_TREE;
> +      res = wi::mod_ceil (arg1, arg2, sign, &overflow);
>        break;
>
>      case ROUND_MOD_EXPR:
> -      res = wi::mod_round (arg1, arg2, sign, &overflow);
> -      if (overflow)
> +      if (arg2 == 0)
>      return NULL_TREE;
> +      res = wi::mod_round (arg1, arg2, sign, &overflow);
>        break;
>
>      case MIN_EXPR:
>
> ================
>
> On 11/20/2013 06:31 PM, Kenneth Zadeck wrote:
>>
>> On 11/13/2013 04:59 AM, Richard Biener wrote:
>>>
>>> On Tue, Nov 12, 2013 at 5:43 PM, Kenneth Zadeck
>>> <zadeck@naturalbridge.com> wrote:
>>>>
>>>> On 11/12/2013 11:27 AM, Joseph S. Myers wrote:
>>>>>
>>>>> On Tue, 12 Nov 2013, Kenneth Zadeck wrote:
>>>>>
>>>>>> Richi,
>>>>>>
>>>>>> i am having a little trouble putting this back the way that you want.
>>>>>> The
>>>>>> issue is rem.
>>>>>> what is supposed to happen for INT_MIN % -1?
>>>>>>
>>>>>> I would assume because i am failing the last case of
>>>>>> gcc.dg/c90-const-expr-8.c
>>>>>> that INT_MIN %-1 should not overflow even if INT_MIN / -1 does.
>>>>>> however,
>>>>>
>>>>> Given the conclusion in C11 that a%b should be considered undefined if
>>>>> a/b
>>>>> is not representable, I think it's reasonable to say INT_MIN % -1
>>>>> *should*
>>>>> be considered to overflow (for all C standard versions) (and bug 30484
>>>>> is
>>>>> only a bug for -fwrapv).
>>>>>
>>>> however, my local question is what do we want the api to be
>>>> int-const-binop-1?    The existing behavior seems to be that at least
>>>> for
>>>> common modes this function silently returns 0 and it is up to the front
>>>> ends
>>>> to put their own spin on it.
>>>
>>> For wide-int you create 1:1 the behavior of current trunk (if a change of
>>> behavior in TImode is not tested in the testsuite then you can ignore
>>> that).
>>> Whatever change you do to semantics of functions you do separately
>>> from wide-int (preferably first on trunk, or at your choice after the
>>> wide-int
>>> merge).
>>>
>>> For this case in question I'd say a % -1 should return 0, but for
>>> INT_MIN % -1 that 0 should have TREE_OVERFLOW set (and
>>> thus you need to adjust that c90-const-expr-8.c testcase).
>>>
>>> Richard.
>>>
>>>> kenny
>>
>> richi,
>> I have done this exactly as you suggested.   bootstrapped and regression
>> tested on x86-64.
>>
>> 2013-11-20  Kenneth Zadeck  <zadeck@naturalbridge.com>
>>
>>     * fold-const.c
>>     (int_const_binop_1): Make INT_MIN % -1 return 0 with the overflow
>>     bit set.
>>
>>
>> 2013-11-20  Kenneth Zadeck  <zadeck@naturalbridge.com>
>>
>>     * gcc.dg/c90-const-expr-8.c: Look for overflow on INT_MIN % -1.
>>     * gcc.dg/c99-const-expr-8.c: Look for overflow on INT_MIN % -1.
>>
>> ok to commit?
>>
>> kenny
>>
>
diff mbox

Patch

===============
ext/random/hypergeometric_distribution/operators/values.cc (test for 
excess errors)

New tests that PASS:

ext/random/hypergeometric_distribution/operators/values.cc execution test
================

also, the corresponding frag for fold-const.c on the wide-int branch 
will look like
================
Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c    (revision 205224)
+++ gcc/fold-const.c    (working copy)
@@ -1030,51 +1030,51 @@  int_const_binop_1 (enum tree_code code,

      case TRUNC_DIV_EXPR:
      case EXACT_DIV_EXPR:
-      res = wi::div_trunc (arg1, arg2, sign, &overflow);
-      if (overflow)
+      if (arg2 == 0)
      return NULL_TREE;
+      res = wi::div_trunc (arg1, arg2, sign, &overflow);
        break;

      case FLOOR_DIV_EXPR:
-      res = wi::div_floor (arg1, arg2, sign, &overflow);
-      if (overflow)
+      if (arg2 == 0)
      return NULL_TREE;
+      res = wi::div_floor (arg1, arg2, sign, &overflow);
        break;

      case CEIL_DIV_EXPR:
-      res = wi::div_ceil (arg1, arg2, sign, &overflow);
-      if (overflow)
+      if (arg2 == 0)
      return NULL_TREE;
+      res = wi::div_ceil (arg1, arg2, sign, &overflow);
        break;

      case ROUND_DIV_EXPR:
-      res = wi::div_round (arg1, arg2, sign, &overflow);
-      if (overflow)
+      if (arg2 == 0)
      return NULL_TREE;
+      res = wi::div_round (arg1, arg2, sign, &overflow);
        break;

      case TRUNC_MOD_EXPR:
-      res = wi::mod_trunc (arg1, arg2, sign, &overflow);
-      if (overflow)
+      if (arg2 == 0)
      return NULL_TREE;
+      res = wi::mod_trunc (arg1, arg2, sign, &overflow);
        break;

      case FLOOR_MOD_EXPR:
-      res = wi::mod_floor (arg1, arg2, sign, &overflow);
-      if (overflow)
+      if (arg2 == 0)
      return NULL_TREE;
+      res = wi::mod_floor (arg1, arg2, sign, &overflow);
        break;

      case CEIL_MOD_EXPR:
-      res = wi::mod_ceil (arg1, arg2, sign, &overflow);
-      if (overflow)
+      if (arg2 == 0)
      return NULL_TREE;
+      res = wi::mod_ceil (arg1, arg2, sign, &overflow);
        break;

      case ROUND_MOD_EXPR:
-      res = wi::mod_round (arg1, arg2, sign, &overflow);
-      if (overflow)
+      if (arg2 == 0)
      return NULL_TREE;
+      res = wi::mod_round (arg1, arg2, sign, &overflow);
        break;

      case MIN_EXPR: