Re: Some wide-int review comments

[Kenneth Zadeck <zadeck at naturalbridge dot com>]

On 11/11/2013 10:04 AM, Kenneth Zadeck wrote:
> On 11/11/2013 09:42 AM, Richard Biener wrote:
> > On Mon, Nov 11, 2013 at 3:26 PM, Kenneth Zadeck
> > <zadeck@naturalbridge.com> wrote:
> > > On 11/11/2013 06:49 AM, Richard Biener wrote:
> > > > On Sat, Nov 9, 2013 at 3:23 PM, Kenneth Zadeck 
> > > > <zadeck@naturalbridge.com>
> > > > wrote:
> > > > > On 11/08/2013 05:30 AM, Richard Sandiford wrote:
> > > > > >    From tree-vrp.c:
> > > > > >
> > > > > > @@ -1893,6 +1884,10 @@ vrp_int_const_binop (enum tree_code code
> > > > > >         /* If the singed operation wraps then int_const_binop has 
> > > > > > done
> > > > > >            everything we want.  */
> > > > > >         ;
> > > > > > +  /* Signed division of -1/0 overflows and by the time it gets here
> > > > > > +     returns NULL_TREE.  */
> > > > > > +  else if (!res)
> > > > > > +    return NULL_TREE;
> > > > > >       else if ((TREE_OVERFLOW (res)
> > > > > >               && !TREE_OVERFLOW (val1)
> > > > > >               && !TREE_OVERFLOW (val2))
> > > > > >
> > > > > > Why is this case different from trunk?  Or is it a bug-fix 
> > > > > > independent
> > > > > > of wide-int?
> > > > > the api for division is different for wide-int than it was for
> > > > > double-int.
> > > > But this is using a tree API (int_const_binop) that didn't change
> > > > (it returned NULL for / 0 previously).  So what makes us arrive here
> > > > now?  (I agree there is a bug in VRP, but it shouldn't manifest itself
> > > > only on wide-int)
> > > >
> > > > Richard.
> > > My reading of the code is that is that i changed int_const_binop to 
> > > return
> > > null_tree for this case.
> > Trunk has:
> >
> >      case TRUNC_DIV_EXPR:
> >      case FLOOR_DIV_EXPR: case CEIL_DIV_EXPR:
> >      case EXACT_DIV_EXPR:
> >        /* This is a shortcut for a common special case.  */
> >        if (op2.high == 0 && (HOST_WIDE_INT) op2.low > 0
> >            && !TREE_OVERFLOW (arg1)
> >            && !TREE_OVERFLOW (arg2)
> >            && op1.high == 0 && (HOST_WIDE_INT) op1.low >= 0)
> >          {
> >            if (code == CEIL_DIV_EXPR)
> >              op1.low += op2.low - 1;
> >
> >            res.low = op1.low / op2.low, res.high = 0;
> >            break;
> >          }
> >
> >        /* ... fall through ...  */
> >
> >      case ROUND_DIV_EXPR:
> >        if (op2.is_zero ())
> >          return NULL_TREE;
> >
> > so it already returns NULL_TREE on divide by zero.
> I found the reason!!!!  This is one of the many "tree-vrp was not 
> properly tested for TImode bugs."
it is an overstatement to say that this was a timode undertesting 
issue.  I made the change to tree-vrp because of wide-int's treating of 
all divide/rem overflows in a uniform manner AND the fact that there was 
a untested divide by zero hole in tree-vrp.      I most likely never saw 
a true divide by zero pass into that section of code.


> on the trunk, the case 0/(smallest negative number) case will only 
> trigger overflow in TImode.     On the wide-int branch, tree-vrp works 
> at the precision of the operands so overflow is triggered properly for 
> this case.    So for HImode, the trunk produces the a result for 
> 0/0x80 and then force_fit code at the bottom of int_const_binop_1 
> turns this into an overflow tree value rather than a null tree.
>
> on the wide-int branch, this case causes the overflow bit to be 
> returned from the wide-int divide because the overflow case is 
> properly handled for all types and that overflow is turned into 
> null_tree by the wide-int version of int_const_binop_1.
>
> apparently there are no test cases that exercise the true divide by 0 
> case but there are test cases that hit the 0/ largest negative number 
> case for modes smaller than TImode.
>
> Kenny
> > > On the trunk, only rem returns null_tree for divide by 0, on the 
> > > wide int
> > > branch, both div and rem return null tree.
> > >
> > > I know that this is going to bring on a string of questions that i 
> > > do not
> > > remember the answers to as to why i made that change. but
> > > fold-const.c:int_const_binop_1 now returns null_tree and this is just
> > > fallout from that change.
> > >
> > > Kenny
> > > > > > Thanks,
> > > > > > Richard