On Thu, Aug 23, 2018 at 4:50 PM Aldy Hernandez <al...@redhat.com> wrote:
>
>
>
> On 08/23/2018 05:59 AM, Richard Biener wrote:
> > On Wed, Aug 22, 2018 at 11:31 AM Aldy Hernandez <al...@redhat.com> wrote:
> >>
> >>
> >>
> >> On 08/21/2018 05:46 AM, Richard Biener wrote:
> >>> On Wed, Aug 15, 2018 at 3:33 AM Aldy Hernandez <al...@redhat.com> wrote:
> >>
> >>>> Finally, my apologies for including a tiny change to the
> >>>> POINTER_PLUS_EXPR handling code as well. It came about the same set of
> >>>> auditing tests.
> >>>
> >>> Bah, please split up things here ;) I've done a related change there
> >>> yesterday...
> >>>
> >>>>
> >>>> It turns out we can handle POINTER_PLUS_EXPR(~[0,0], [X,Y]) without
> >>>> bailing as VR_VARYING in extract_range_from_binary_expr_1. In doing so,
> >>>> I also noticed that ~[0,0] is not the only non-null. We could also have
> >>>> ~[0,2] and still know that the pointer is not zero. I have adjusted
> >>>> range_is_nonnull accordingly.
> >>>
> >>> But there are other consumers and it would have been better to
> >>> change range_includes_zero_p to do the natural thing (get a VR) and
> >>> then remove range_is_nonnull as redundant if possible.
> >>
> >> Indeed. Cleaning up range_includes_zero_p makes VRP and friends a lot
> >> cleaner. Thanks for the suggestion.
> >>
> >> I lazily avoided cleaning up the division code affected in this patch
> >> too much, since it's going to be superseded by my division changes in
> >> the other patch.
> >>
> >> OK pending tests?
> >
> > -/* Return 1 if [MIN, MAX] includes the value zero, 0 if it does not
> > - include the value zero, -2 if we cannot tell. */
> > +/* Return 1 if *VR includes the value zero, 0 if it does not include
> > + the value zero, or -2 if we cannot tell. */
> >
> > int
> > -range_includes_zero_p (tree min, tree max)
> > +range_includes_zero_p (const value_range *vr)
> > {
> > - tree zero = build_int_cst (TREE_TYPE (min), 0);
> > - return value_inside_range (zero, min, max);
> > + if (vr->type == VR_UNDEFINED || vr->type == VR_VARYING)
> > + return -2;
> > +
> > + tree zero = build_int_cst (TREE_TYPE (vr->min), 0);
> > + if (vr->type == VR_RANGE)
> > + return value_inside_range (zero, vr->min, vr->max);
> > + else
> > + return !value_inside_range (zero, vr->min, vr->max);
> > }
> >
> > please make it return a bool. VR_VARYING means the range does
> > include zero. For VR_UNDEFINED we could say it doesn't or we choose
> > to not possibly optimize and thus return true (just do that for now).
> > That's because VR_VARYING is [-INF, INF] and VR_UNDEFINED is
> > an empty range.
>
> Done.
>
> >
> > I suppose the -2 for we cannot tell was for the case of symbolic
> > ranges where again we can conservatively return true
> > (and your return !value_inside_range for VR_ANTI_RANGE botched
> > the tri-state return value anyways...).
>
> Bah! I sure botched up the anti range return. That was the last
> bootstrap failure I was investigating. You'd be amazed how many things
> mysteriously fail when things like ~[SYMBOL, SYMBOL] are assumed to be
> non-zero.
>
> Thanks for spotting this. Bootstrap is happy now :-P.
>
> >
> > So, can you please rework that?
>
> Sure can!
@@ -1407,7 +1407,10 @@ extract_range_from_binary_expr_1 (value_range *vr,
&& code != POINTER_PLUS_EXPR
&& (vr0.type == VR_VARYING
|| vr1.type == VR_VARYING
- || vr0.type != vr1.type
+ || (vr0.type != vr1.type
+ /* We can handle POINTER_PLUS_EXPR(~[0,0], [x,y]) below,
+ even though we have differing range kinds. */
+ && code != POINTER_PLUS_EXPR)
|| symbolic_range_p (&vr0)
|| symbolic_range_p (&vr1)))
{
is redundant now (spot the code != POINTER_PLUS_EXPR check at the
beginning of context)
OK with this hunk removed.
Thanks,
Richard.
> Attached.
>
